Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Aug 2, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

16 changes: 3 additions & 13 deletions crates/paths/src/lib.rs
Original file line number Diff line number Diff line change
@@ -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::{
Expand All @@ -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);

Expand Down Expand Up @@ -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> {
Expand Down
14 changes: 5 additions & 9 deletions crates/project-model/src/build_scripts.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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}");

Expand All @@ -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));
}
}
Expand Down
7 changes: 5 additions & 2 deletions crates/project-model/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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::{
Expand Down Expand Up @@ -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()
}
}
Expand Down
3 changes: 3 additions & 0 deletions crates/rust-analyzer/src/bin/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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},
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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<_>>()
})
Expand Down
3 changes: 2 additions & 1 deletion crates/rust-analyzer/src/cli/rustc_tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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::{
Expand Down Expand Up @@ -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() };
Expand Down
71 changes: 18 additions & 53 deletions crates/rust-analyzer/src/config.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3450,15 +3450,15 @@ 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)
}
}

#[test]
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 {
Expand All @@ -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!({
Expand All @@ -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();

Expand All @@ -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();

Expand All @@ -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!({
Expand All @@ -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!({
Expand All @@ -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();

Expand Down
3 changes: 3 additions & 0 deletions crates/rust-analyzer/src/handlers/notification.rs
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ use lsp_types::{
DidChangeWatchedFilesParams, DidChangeWorkspaceFoldersParams, DidCloseTextDocumentParams,
DidOpenTextDocumentParams, DidSaveTextDocumentParams, WorkDoneProgressCancelParams,
};
use paths::Utf8PathBuf;
use triomphe::Arc;
use vfs::{AbsPathBuf, ChangeKind, VfsPath};

Expand Down Expand Up @@ -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);
}
Expand All @@ -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);

Expand Down
9 changes: 6 additions & 3 deletions crates/rust-analyzer/src/handlers/request.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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() {
Expand Down
30 changes: 24 additions & 6 deletions crates/rust-analyzer/src/integrated_benchmarks.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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}"))
};

Expand Down Expand Up @@ -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}"))
};

Expand Down Expand Up @@ -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}"))
};

Expand Down
Loading