Auto merge of #17770 - Veykril:path-try-from, r=Veykril

internal: Remove AbsPathBuf::TryFrom impl that checks too many things at once

https://github.com/rust-lang/rust-analyzer/pull/16889#discussion_r1590993282

Cargo.lock

@@ -1987,6 +1987,7 @@ name = "test-utils"
 version = "0.0.0"
 dependencies = [
  "dissimilar",
+ "paths",
  "profile",
  "rustc-hash",
  "stdx",

crates/paths/src/lib.rs

@@ -1,4 +1,4 @@
-//! Thin wrappers around `std::path`/`camino::path`, distinguishing between absolute and
+//! Thin wrappers around [`camino::path`], distinguishing between absolute and
 //! relative paths.
 
 use std::{
@@ -8,9 +8,9 @@ use std::{
     path::{Path, PathBuf},
 };
 
-pub use camino::*;
+pub use camino::{Utf8Component, Utf8Components, Utf8Path, Utf8PathBuf, Utf8Prefix};
 
-/// Wrapper around an absolute [`Utf8PathBuf`].
+/// A [`Utf8PathBuf`] that is guaranteed to be absolute.
 #[derive(Debug, Clone, Ord, PartialOrd, Eq, Hash)]
 pub struct AbsPathBuf(Utf8PathBuf);
 
@@ -73,16 +73,6 @@ impl TryFrom<Utf8PathBuf> for AbsPathBuf {
     }
 }
 
-impl TryFrom<PathBuf> for AbsPathBuf {
-    type Error = PathBuf;
-    fn try_from(path_buf: PathBuf) -> Result<AbsPathBuf, PathBuf> {
-        if !path_buf.is_absolute() {
-            return Err(path_buf);
-        }
-        Ok(AbsPathBuf(Utf8PathBuf::from_path_buf(path_buf)?))
-    }
-}
-
 impl TryFrom<&str> for AbsPathBuf {
     type Error = Utf8PathBuf;
     fn try_from(path: &str) -> Result<AbsPathBuf, Utf8PathBuf> {

crates/project-model/src/build_scripts.rs

@@ -6,17 +6,12 @@
 //! This module implements this second part. We use "build script" terminology
 //! here, but it covers procedural macros as well.
 
-use std::{
-    cell::RefCell,
-    io, mem,
-    path::{self, PathBuf},
-    process::Command,
-};
+use std::{cell::RefCell, io, mem, path, process::Command};
 
 use cargo_metadata::{camino::Utf8Path, Message};
 use itertools::Itertools;
 use la_arena::ArenaMap;
-use paths::{AbsPath, AbsPathBuf};
+use paths::{AbsPath, AbsPathBuf, Utf8PathBuf};
 use rustc_hash::{FxHashMap, FxHashSet};
 use serde::Deserialize;
 use toolchain::Tool;
@@ -423,7 +418,7 @@ impl WorkspaceBuildScripts {
                 utf8_stdout(cmd)
             })()?;
 
-            let target_libdir = AbsPathBuf::try_from(PathBuf::from(target_libdir))
+            let target_libdir = AbsPathBuf::try_from(Utf8PathBuf::from(target_libdir))
                 .map_err(|_| anyhow::format_err!("target-libdir was not an absolute path"))?;
             tracing::info!("Loading rustc proc-macro paths from {target_libdir}");
 
@@ -435,7 +430,8 @@ impl WorkspaceBuildScripts {
                         let extension = path.extension()?;
                         if extension == std::env::consts::DLL_EXTENSION {
                             let name = path.file_stem()?.to_str()?.split_once('-')?.0.to_owned();
-                            let path = AbsPathBuf::try_from(path).ok()?;
+                            let path = AbsPathBuf::try_from(Utf8PathBuf::from_path_buf(path).ok()?)
+                                .ok()?;
                             return Some((name, path));
                         }
                     }

crates/project-model/src/lib.rs

@@ -37,7 +37,7 @@ use std::{
 };
 
 use anyhow::{bail, format_err, Context};
-use paths::{AbsPath, AbsPathBuf};
+use paths::{AbsPath, AbsPathBuf, Utf8PathBuf};
 use rustc_hash::FxHashSet;
 
 pub use crate::{
@@ -132,8 +132,11 @@ impl ProjectManifest {
                 .filter_map(Result::ok)
                 .map(|it| it.path().join("Cargo.toml"))
                 .filter(|it| it.exists())
+                .map(Utf8PathBuf::from_path_buf)
+                .filter_map(Result::ok)
                 .map(AbsPathBuf::try_from)
-                .filter_map(|it| it.ok()?.try_into().ok())
+                .filter_map(Result::ok)
+                .filter_map(|it| it.try_into().ok())
                 .collect()
         }
     }

crates/rust-analyzer/src/bin/main.rs

@@ -14,6 +14,7 @@ use std::{env, fs, path::PathBuf, process::ExitCode, sync::Arc};
 
 use anyhow::Context;
 use lsp_server::Connection;
+use paths::Utf8PathBuf;
 use rust_analyzer::{
     cli::flags,
     config::{Config, ConfigChange, ConfigErrors},
@@ -189,6 +190,7 @@ fn run_server() -> anyhow::Result<()> {
     let root_path = match root_uri
         .and_then(|it| it.to_file_path().ok())
         .map(patch_path_prefix)
+        .and_then(|it| Utf8PathBuf::from_path_buf(it).ok())
         .and_then(|it| AbsPathBuf::try_from(it).ok())
     {
         Some(it) => it,
@@ -218,6 +220,7 @@ fn run_server() -> anyhow::Result<()> {
                 .into_iter()
                 .filter_map(|it| it.uri.to_file_path().ok())
                 .map(patch_path_prefix)
+                .filter_map(|it| Utf8PathBuf::from_path_buf(it).ok())
                 .filter_map(|it| AbsPathBuf::try_from(it).ok())
                 .collect::<Vec<_>>()
         })

crates/rust-analyzer/src/cli/rustc_tests.rs

@@ -8,6 +8,7 @@ use std::{cell::RefCell, fs::read_to_string, panic::AssertUnwindSafe, path::Path
 use hir::{ChangeWithProcMacros, Crate};
 use ide::{AnalysisHost, DiagnosticCode, DiagnosticsConfig};
 use itertools::Either;
+use paths::Utf8PathBuf;
 use profile::StopWatch;
 use project_model::target_data_layout::RustcDataLayoutConfig;
 use project_model::{
@@ -64,7 +65,7 @@ impl Tester {
     fn new() -> Result<Self> {
         let mut path = std::env::temp_dir();
         path.push("ra-rustc-test.rs");
-        let tmp_file = AbsPathBuf::try_from(path).unwrap();
+        let tmp_file = AbsPathBuf::try_from(Utf8PathBuf::from_path_buf(path).unwrap()).unwrap();
         std::fs::write(&tmp_file, "")?;
         let cargo_config =
             CargoConfig { sysroot: Some(RustLibSource::Discover), ..Default::default() };

crates/rust-analyzer/src/config.rs

@@ -3450,7 +3450,7 @@ mod tests {
         let s = remove_ws(&schema);
         if !p.contains(&s) {
             package_json.replace_range(start..end, &schema);
-            ensure_file_contents(&package_json_path, &package_json)
+            ensure_file_contents(package_json_path.as_std_path(), &package_json)
         }
     }
 
@@ -3458,7 +3458,7 @@ mod tests {
     fn generate_config_documentation() {
         let docs_path = project_root().join("docs/user/generated_config.adoc");
         let expected = FullConfigInput::manual();
-        ensure_file_contents(&docs_path, &expected);
+        ensure_file_contents(docs_path.as_std_path(), &expected);
     }
 
     fn remove_ws(text: &str) -> String {
@@ -3467,13 +3467,8 @@ mod tests {
 
     #[test]
     fn proc_macro_srv_null() {
-        let mut config = Config::new(
-            AbsPathBuf::try_from(project_root()).unwrap(),
-            Default::default(),
-            vec![],
-            None,
-            None,
-        );
+        let mut config =
+            Config::new(AbsPathBuf::assert(project_root()), Default::default(), vec![], None, None);
 
         let mut change = ConfigChange::default();
         change.change_client_config(serde_json::json!({
@@ -3487,32 +3482,22 @@ mod tests {
 
     #[test]
     fn proc_macro_srv_abs() {
-        let mut config = Config::new(
-            AbsPathBuf::try_from(project_root()).unwrap(),
-            Default::default(),
-            vec![],
-            None,
-            None,
-        );
+        let mut config =
+            Config::new(AbsPathBuf::assert(project_root()), Default::default(), vec![], None, None);
         let mut change = ConfigChange::default();
         change.change_client_config(serde_json::json!({
         "procMacro" : {
-            "server": project_root().display().to_string(),
+            "server": project_root().to_string(),
         }}));
 
         (config, _, _) = config.apply_change(change);
-        assert_eq!(config.proc_macro_srv(), Some(AbsPathBuf::try_from(project_root()).unwrap()));
+        assert_eq!(config.proc_macro_srv(), Some(AbsPathBuf::assert(project_root())));
     }
 
     #[test]
     fn proc_macro_srv_rel() {
-        let mut config = Config::new(
-            AbsPathBuf::try_from(project_root()).unwrap(),
-            Default::default(),
-            vec![],
-            None,
-            None,
-        );
+        let mut config =
+            Config::new(AbsPathBuf::assert(project_root()), Default::default(), vec![], None, None);
 
         let mut change = ConfigChange::default();
 
@@ -3531,13 +3516,8 @@ mod tests {
 
     #[test]
     fn cargo_target_dir_unset() {
-        let mut config = Config::new(
-            AbsPathBuf::try_from(project_root()).unwrap(),
-            Default::default(),
-            vec![],
-            None,
-            None,
-        );
+        let mut config =
+            Config::new(AbsPathBuf::assert(project_root()), Default::default(), vec![], None, None);
 
         let mut change = ConfigChange::default();
 
@@ -3554,13 +3534,8 @@ mod tests {
 
     #[test]
     fn cargo_target_dir_subdir() {
-        let mut config = Config::new(
-            AbsPathBuf::try_from(project_root()).unwrap(),
-            Default::default(),
-            vec![],
-            None,
-            None,
-        );
+        let mut config =
+            Config::new(AbsPathBuf::assert(project_root()), Default::default(), vec![], None, None);
 
         let mut change = ConfigChange::default();
         change.change_client_config(serde_json::json!({
@@ -3577,13 +3552,8 @@ mod tests {
 
     #[test]
     fn cargo_target_dir_relative_dir() {
-        let mut config = Config::new(
-            AbsPathBuf::try_from(project_root()).unwrap(),
-            Default::default(),
-            vec![],
-            None,
-            None,
-        );
+        let mut config =
+            Config::new(AbsPathBuf::assert(project_root()), Default::default(), vec![], None, None);
 
         let mut change = ConfigChange::default();
         change.change_client_config(serde_json::json!({
@@ -3603,13 +3573,8 @@ mod tests {
 
     #[test]
     fn toml_unknown_key() {
-        let config = Config::new(
-            AbsPathBuf::try_from(project_root()).unwrap(),
-            Default::default(),
-            vec![],
-            None,
-            None,
-        );
+        let config =
+            Config::new(AbsPathBuf::assert(project_root()), Default::default(), vec![], None, None);
 
         let mut change = ConfigChange::default();
 

crates/rust-analyzer/src/handlers/notification.rs

@@ -9,6 +9,7 @@ use lsp_types::{
     DidChangeWatchedFilesParams, DidChangeWorkspaceFoldersParams, DidCloseTextDocumentParams,
     DidOpenTextDocumentParams, DidSaveTextDocumentParams, WorkDoneProgressCancelParams,
 };
+use paths::Utf8PathBuf;
 use triomphe::Arc;
 use vfs::{AbsPathBuf, ChangeKind, VfsPath};
 
@@ -240,6 +241,7 @@ pub(crate) fn handle_did_change_workspace_folders(
 
     for workspace in params.event.removed {
         let Ok(path) = workspace.uri.to_file_path() else { continue };
+        let Ok(path) = Utf8PathBuf::from_path_buf(path) else { continue };
         let Ok(path) = AbsPathBuf::try_from(path) else { continue };
         config.remove_workspace(&path);
     }
@@ -249,6 +251,7 @@ pub(crate) fn handle_did_change_workspace_folders(
         .added
         .into_iter()
         .filter_map(|it| it.uri.to_file_path().ok())
+        .filter_map(|it| Utf8PathBuf::from_path_buf(it).ok())
         .filter_map(|it| AbsPathBuf::try_from(it).ok());
     config.add_workspaces(added);
 

crates/rust-analyzer/src/handlers/request.rs

@@ -781,9 +781,12 @@ pub(crate) fn handle_parent_module(
     if let Ok(file_path) = &params.text_document.uri.to_file_path() {
         if file_path.file_name().unwrap_or_default() == "Cargo.toml" {
             // search workspaces for parent packages or fallback to workspace root
-            let abs_path_buf = match AbsPathBuf::try_from(file_path.to_path_buf()).ok() {
-                Some(abs_path_buf) => abs_path_buf,
-                None => return Ok(None),
+            let abs_path_buf = match Utf8PathBuf::from_path_buf(file_path.to_path_buf())
+                .ok()
+                .map(AbsPathBuf::try_from)
+            {
+                Some(Ok(abs_path_buf)) => abs_path_buf,
+                _ => return Ok(None),
             };
 
             let manifest_path = match ManifestPath::try_from(abs_path_buf).ok() {

crates/rust-analyzer/src/integrated_benchmarks.rs

@@ -46,13 +46,19 @@ fn integrated_highlighting_benchmark() {
 
     let (db, vfs, _proc_macro) = {
         let _it = stdx::timeit("workspace loading");
-        load_workspace_at(&workspace_to_load, &cargo_config, &load_cargo_config, &|_| {}).unwrap()
+        load_workspace_at(
+            workspace_to_load.as_std_path(),
+            &cargo_config,
+            &load_cargo_config,
+            &|_| {},
+        )
+        .unwrap()
     };
     let mut host = AnalysisHost::with_database(db);
 
     let file_id = {
         let file = workspace_to_load.join(file);
-        let path = VfsPath::from(AbsPathBuf::assert_utf8(file));
+        let path = VfsPath::from(AbsPathBuf::assert(file));
         vfs.file_id(&path).unwrap_or_else(|| panic!("can't find virtual file for {path}"))
     };
 
@@ -106,13 +112,19 @@ fn integrated_completion_benchmark() {
 
     let (db, vfs, _proc_macro) = {
         let _it = stdx::timeit("workspace loading");
-        load_workspace_at(&workspace_to_load, &cargo_config, &load_cargo_config, &|_| {}).unwrap()
+        load_workspace_at(
+            workspace_to_load.as_std_path(),
+            &cargo_config,
+            &load_cargo_config,
+            &|_| {},
+        )
+        .unwrap()
     };
     let mut host = AnalysisHost::with_database(db);
 
     let file_id = {
         let file = workspace_to_load.join(file);
-        let path = VfsPath::from(AbsPathBuf::assert_utf8(file));
+        let path = VfsPath::from(AbsPathBuf::assert(file));
         vfs.file_id(&path).unwrap_or_else(|| panic!("can't find virtual file for {path}"))
     };
 
@@ -274,13 +286,19 @@ fn integrated_diagnostics_benchmark() {
 
     let (db, vfs, _proc_macro) = {
         let _it = stdx::timeit("workspace loading");
-        load_workspace_at(&workspace_to_load, &cargo_config, &load_cargo_config, &|_| {}).unwrap()
+        load_workspace_at(
+            workspace_to_load.as_std_path(),
+            &cargo_config,
+            &load_cargo_config,
+            &|_| {},
+        )
+        .unwrap()
     };
     let mut host = AnalysisHost::with_database(db);
 
     let file_id = {
         let file = workspace_to_load.join(file);
-        let path = VfsPath::from(AbsPathBuf::assert_utf8(file));
+        let path = VfsPath::from(AbsPathBuf::assert(file));
         vfs.file_id(&path).unwrap_or_else(|| panic!("can't find virtual file for {path}"))
     };
 

crates/rust-analyzer/src/lsp/from_proto.rs

@@ -2,6 +2,7 @@
 use anyhow::format_err;
 use ide::{Annotation, AnnotationKind, AssistKind, LineCol};
 use ide_db::{line_index::WideLineCol, FileId, FilePosition, FileRange};
+use paths::Utf8PathBuf;
 use syntax::{TextRange, TextSize};
 use vfs::AbsPathBuf;
 
@@ -13,7 +14,7 @@ use crate::{
 
 pub(crate) fn abs_path(url: &lsp_types::Url) -> anyhow::Result<AbsPathBuf> {
     let path = url.to_file_path().map_err(|()| anyhow::format_err!("url is not a file"))?;
-    Ok(AbsPathBuf::try_from(path).unwrap())
+    Ok(AbsPathBuf::try_from(Utf8PathBuf::from_path_buf(path).unwrap()).unwrap())
 }
 
 pub(crate) fn vfs_path(url: &lsp_types::Url) -> anyhow::Result<vfs::VfsPath> {

crates/syntax/src/tests.rs

@@ -79,7 +79,7 @@ fn self_hosting_parsing() {
     let crates_dir = project_root().join("crates");
 
     let mut files = Vec::new();
-    let mut work = vec![crates_dir.to_path_buf()];
+    let mut work = vec![crates_dir.into_std_path_buf()];
     while let Some(dir) = work.pop() {
         for entry in dir.read_dir().unwrap() {
             let entry = entry.unwrap();
@@ -127,7 +127,7 @@ fn self_hosting_parsing() {
 }
 
 fn test_data_dir() -> PathBuf {
-    project_root().join("crates/syntax/test_data")
+    project_root().into_std_path_buf().join("crates/syntax/test_data")
 }
 
 fn assert_errors_are_present(errors: &[SyntaxError], path: &Path) {

crates/test-utils/Cargo.toml

@@ -18,6 +18,7 @@ text-size.workspace = true
 tracing.workspace = true
 rustc-hash.workspace = true
 
+paths.workspace = true
 stdx.workspace = true
 profile.workspace = true
 

crates/test-utils/src/lib.rs

@@ -18,6 +18,7 @@ use std::{
     path::{Path, PathBuf},
 };
 
+use paths::Utf8PathBuf;
 use profile::StopWatch;
 use stdx::is_ci;
 use text_size::{TextRange, TextSize};
@@ -402,9 +403,10 @@ pub fn skip_slow_tests() -> bool {
 }
 
 /// Returns the path to the root directory of `rust-analyzer` project.
-pub fn project_root() -> PathBuf {
+pub fn project_root() -> Utf8PathBuf {
     let dir = env!("CARGO_MANIFEST_DIR");
-    PathBuf::from(dir).parent().unwrap().parent().unwrap().to_owned()
+    Utf8PathBuf::from_path_buf(PathBuf::from(dir).parent().unwrap().parent().unwrap().to_owned())
+        .unwrap()
 }
 
 pub fn format_diff(chunks: Vec<dissimilar::Chunk<'_>>) -> String {

crates/vfs-notify/src/lib.rs

@@ -14,7 +14,7 @@ use std::{
 
 use crossbeam_channel::{never, select, unbounded, Receiver, Sender};
 use notify::{Config, RecommendedWatcher, RecursiveMode, Watcher};
-use paths::{AbsPath, AbsPathBuf};
+use paths::{AbsPath, AbsPathBuf, Utf8PathBuf};
 use vfs::loader;
 use walkdir::WalkDir;
 
@@ -145,7 +145,12 @@ impl NotifyActor {
                         let files = event
                             .paths
                             .into_iter()
-                            .map(|path| AbsPathBuf::try_from(path).unwrap())
+                            .filter_map(|path| {
+                                Some(
+                                    AbsPathBuf::try_from(Utf8PathBuf::from_path_buf(path).ok()?)
+                                        .expect("path is absolute"),
+                                )
+                            })
                             .filter_map(|path| {
                                 let meta = fs::metadata(&path).ok()?;
                                 if meta.file_type().is_dir()
@@ -220,7 +225,10 @@ impl NotifyActor {
                         let depth = entry.depth();
                         let is_dir = entry.file_type().is_dir();
                         let is_file = entry.file_type().is_file();
-                        let abs_path = AbsPathBuf::try_from(entry.into_path()).ok()?;
+                        let abs_path = AbsPathBuf::try_from(
+                            Utf8PathBuf::from_path_buf(entry.into_path()).ok()?,
+                        )
+                        .ok()?;
                         if depth < 2 && is_dir {
                             self.send(make_message(abs_path.clone()));
                         }