diff --git a/app/buck2_server/src/lsp.rs b/app/buck2_server/src/lsp.rs index b32bd3bfa327..dff81ae7abfb 100644 --- a/app/buck2_server/src/lsp.rs +++ b/app/buck2_server/src/lsp.rs @@ -905,7 +905,8 @@ mod tests { async fn lookup_function(location: &ImportPath) -> buck2_error::Result { if location == &ImportPath::testing_new(P) { Ok(LspUrl::try_from(Url::parse( - "file:///usr/local/dir/prelude.bzl", + // Make sure we use a Url which is an absolute path on Linux and Windows + "file:////c:/usr/local/dir/prelude.bzl", )?)?) } else { Err(buck2_error::buck2_error!([], "Unknown path {}", location)) @@ -924,7 +925,7 @@ mod tests { cache.url_for_symbol("native_function2").unwrap() ); assert_eq!( - &LspUrl::try_from(Url::parse("file:/usr/local/dir/prelude.bzl")?)?, + &LspUrl::try_from(Url::parse("file:/c:/usr/local/dir/prelude.bzl")?)?, cache.url_for_symbol("prelude_function").unwrap() ); diff --git a/starlark-rust/starlark_lsp/BUCK b/starlark-rust/starlark_lsp/BUCK index ef378fae15f8..90c2547f471c 100644 --- a/starlark-rust/starlark_lsp/BUCK +++ b/starlark-rust/starlark_lsp/BUCK @@ -9,23 +9,10 @@ rust_library( "--cfg=rust_nightly", ], test_deps = [ + "fbsource//third-party/rust:maplit", "fbsource//third-party/rust:regex", "fbsource//third-party/rust:textwrap", ], - test_os_deps = [ - ( - "linux", - [ - "fbsource//third-party/rust:maplit", - ], - ), - ( - "macos", - [ - "fbsource//third-party/rust:maplit", - ], - ), - ], deps = [ "fbsource//third-party/rust:anyhow", "fbsource//third-party/rust:derivative", diff --git a/starlark-rust/starlark_lsp/Cargo.toml b/starlark-rust/starlark_lsp/Cargo.toml index 940103dea876..2c9d32ab636c 100644 --- a/starlark-rust/starlark_lsp/Cargo.toml +++ b/starlark-rust/starlark_lsp/Cargo.toml @@ -29,8 +29,6 @@ starlark = { version = "0.12.0", path = "../starlark" } starlark_syntax = { version = "0.12.0", path = "../starlark_syntax" } [dev-dependencies] +maplit = "1.0.2" regex = "1.5.4" textwrap = "0.11" - -[target.'cfg(not(windows))'.dev-dependencies] -maplit = "1.0.2" diff --git a/starlark-rust/starlark_lsp/src/definition.rs b/starlark-rust/starlark_lsp/src/definition.rs index ad0ce3053154..e516f6f07a3e 100644 --- a/starlark-rust/starlark_lsp/src/definition.rs +++ b/starlark-rust/starlark_lsp/src/definition.rs @@ -736,7 +736,6 @@ pub(crate) mod helpers { )?)) } - #[cfg(not(windows))] pub(crate) fn program(&self) -> String { self.program.clone() } diff --git a/starlark-rust/starlark_lsp/src/lib.rs b/starlark-rust/starlark_lsp/src/lib.rs index 496a94f93368..ba2e6ee6d02c 100644 --- a/starlark-rust/starlark_lsp/src/lib.rs +++ b/starlark-rust/starlark_lsp/src/lib.rs @@ -31,5 +31,5 @@ pub(crate) mod inspect; pub(crate) mod loaded; pub mod server; mod symbols; -#[cfg(all(test, not(windows)))] +#[cfg(test)] mod test; diff --git a/starlark-rust/starlark_lsp/src/server.rs b/starlark-rust/starlark_lsp/src/server.rs index 19cf574682cd..58325890143b 100644 --- a/starlark-rust/starlark_lsp/src/server.rs +++ b/starlark-rust/starlark_lsp/src/server.rs @@ -142,6 +142,8 @@ pub enum LspUrlError { /// For some reason the PathBuf/Url in the LspUrl could not be converted back to a URL. #[error("`{}` could not be converted back to a URL", .0)] Unparsable(LspUrl), + #[error("invalid URL for file:// schema (possibly not absolute?): `{}`", .0)] + InvalidFileUrl(Url), } /// A URL that represents the two types (plus an "Other") of URIs that are supported. @@ -198,9 +200,12 @@ impl TryFrom for LspUrl { fn try_from(url: Url) -> Result { match url.scheme() { "file" => { - let path = PathBuf::from(url.path()); - if path.to_string_lossy().starts_with('/') { - Ok(Self::File(path)) + let file_path = PathBuf::from( + url.to_file_path() + .map_err(|_| LspUrlError::InvalidFileUrl(url.clone()))?, + ); + if file_path.is_absolute() { + Ok(Self::File(file_path)) } else { Err(LspUrlError::NotAbsolute(url)) } @@ -432,14 +437,14 @@ impl Backend { } fn validate(&self, uri: Url, version: Option, text: String) -> anyhow::Result<()> { - let uri = uri.try_into()?; - let eval_result = self.context.parse_file_with_contents(&uri, text); + let lsp_url = uri.clone().try_into()?; + let eval_result = self.context.parse_file_with_contents(&lsp_url, text); if let Some(ast) = eval_result.ast { let module = Arc::new(LspModule::new(ast)); let mut last_valid_parse = self.last_valid_parse.write().unwrap(); - last_valid_parse.insert(uri.clone(), module); + last_valid_parse.insert(lsp_url, module); } - self.publish_diagnostics(uri.try_into()?, eval_result.diagnostics, version); + self.publish_diagnostics(uri, eval_result.diagnostics, version); Ok(()) } @@ -1361,9 +1366,7 @@ where } } -// TODO(nmj): Some of the windows tests get a bit flaky, especially around -// some paths. Revisit later. -#[cfg(all(test, not(windows)))] +#[cfg(test)] mod tests { use std::path::Path; use std::path::PathBuf; @@ -1469,6 +1472,16 @@ mod tests { Url::from_file_path(PathBuf::from("/tmp").join(rel_path)).unwrap() } + // Converts PathBuf to string that can be used in starlark load statements within "" quotes. + // Replaces \ with / (for Windows paths). + fn path_to_load_string(p: &Path) -> String { + p.to_str().unwrap().replace('\\', "/") + } + + fn uri_to_load_string(uri: &Url) -> String { + path_to_load_string(&uri.to_file_path().unwrap()) + } + #[test] fn sends_empty_goto_definition_on_nonexistent_file() -> anyhow::Result<()> { if is_wasm() { @@ -1574,7 +1587,7 @@ mod tests { baz() "#, ) - .replace("{load}", bar_uri.path()) + .replace("{load}", &uri_to_load_string(&bar_uri)) .trim() .to_owned(); let bar_contents = "def baz():\n pass"; @@ -1590,7 +1603,7 @@ mod tests { let mut server = TestServer::new()?; // Initialize with "junk" on disk so that we make sure we're using the contents from the // client (potentially indicating an unsaved, modified file) - server.set_file_contents(PathBuf::from(bar_uri.path()), "some_symbol = 1".to_owned())?; + server.set_file_contents(&bar_uri, "some_symbol = 1".to_owned())?; server.open_file(foo_uri.clone(), foo.program())?; server.open_file(bar_uri, bar.program())?; @@ -1623,9 +1636,10 @@ mod tests { baz() "#, ) - .replace("{load}", bar_uri.path()) + .replace("{load}", &uri_to_load_string(&bar_uri)) .trim() .to_owned(); + eprintln!("foo_contents: {}", foo_contents); let bar_contents = "def baz():\n pass"; let foo = FixtureWithRanges::from_fixture(foo_uri.path(), &foo_contents)?; let bar = FixtureWithRanges::from_fixture(bar_uri.path(), bar_contents)?; @@ -1638,7 +1652,7 @@ mod tests { let mut server = TestServer::new()?; server.open_file(foo_uri.clone(), foo.program())?; - server.set_file_contents(PathBuf::from(bar_uri.path()), bar.program())?; + server.set_file_contents(&bar_uri, bar.program())?; let goto_definition = goto_definition_request( &mut server, @@ -1683,7 +1697,7 @@ mod tests { let mut server = TestServer::new()?; server.open_file(foo_uri.clone(), foo.program())?; - server.set_file_contents(PathBuf::from(bar_uri.path()), bar.program())?; + server.set_file_contents(&bar_uri, bar.program())?; let goto_definition = goto_definition_request( &mut server, @@ -1769,7 +1783,7 @@ mod tests { let mut server = TestServer::new()?; server.open_file(foo_uri.clone(), foo.program())?; - server.set_file_contents(PathBuf::from(bar_uri.path()), bar.program())?; + server.set_file_contents(&bar_uri, bar.program())?; let goto_definition = goto_definition_request( &mut server, @@ -1800,7 +1814,7 @@ mod tests { baz() "#, ) - .replace("{load}", bar_uri.path()) + .replace("{load}", &uri_to_load_string(&bar_uri)) .trim() .to_owned(); let bar_contents = "def baz():\n pass"; @@ -1815,7 +1829,7 @@ mod tests { let mut server = TestServer::new()?; server.open_file(foo_uri.clone(), foo.program())?; - server.set_file_contents(PathBuf::from(bar_uri.path()), bar.program())?; + server.set_file_contents(&bar_uri, bar.program())?; let goto_definition = goto_definition_request( &mut server, @@ -1860,7 +1874,7 @@ mod tests { let mut server = TestServer::new()?; server.open_file(foo_uri.clone(), foo.program())?; - server.set_file_contents(PathBuf::from(bar_uri.path()), bar.program())?; + server.set_file_contents(&bar_uri, bar.program())?; let goto_definition = goto_definition_request( &mut server, @@ -1884,12 +1898,13 @@ mod tests { let foo_uri = temp_file_uri("foo.star"); let bar_uri = temp_file_uri("bar.star"); - let bar_load_string = Path::new(bar_uri.path()) + let load_path = bar_uri + .to_file_path() + .unwrap() .parent() .unwrap() - .join("bar.star") - .display() - .to_string(); + .join("bar.star"); + let bar_load_string = path_to_load_string(&load_path); let foo_contents = dedent( r#" @@ -2141,7 +2156,7 @@ mod tests { let mut server = TestServer::new()?; server.open_file(foo_uri.clone(), foo.program())?; - server.set_file_contents(PathBuf::from(bar_uri.path()), bar.program())?; + server.set_file_contents(&bar_uri, bar.program())?; server.mkdir(dir1_uri.clone()); server.mkdir(dir2_uri.clone()); @@ -2425,7 +2440,7 @@ mod tests { loaded.y "#, ) - .replace("{load}", bar_uri.path()) + .replace("{load}", &uri_to_load_string(&bar_uri)) .trim() .to_owned(); diff --git a/starlark-rust/starlark_lsp/src/test.rs b/starlark-rust/starlark_lsp/src/test.rs index 36f53d346e23..0034e02eeb73 100644 --- a/starlark-rust/starlark_lsp/src/test.rs +++ b/starlark-rust/starlark_lsp/src/test.rs @@ -75,14 +75,6 @@ use crate::server::LspServerSettings; use crate::server::LspUrl; use crate::server::StringLiteralResult; -/// Get the path from a URL, trimming off things like the leading slash that gets -/// appended in some windows test environments. -#[cfg(windows)] -fn get_path_from_uri(uri: &str) -> PathBuf { - PathBuf::from(uri.trim_start_match('/')) -} - -#[cfg(not(windows))] fn get_path_from_uri(uri: &str) -> PathBuf { PathBuf::from(uri) } @@ -662,9 +654,11 @@ impl TestServer { Ok(()) } - /// Set the file contents that `get_load_contents()` will return. The path must be absolute. - pub fn set_file_contents(&self, path: PathBuf, contents: String) -> anyhow::Result<()> { - let path = get_path_from_uri(&format!("{}", path.display())); + /// Set the file contents that `get_load_contents()` will return. + pub fn set_file_contents(&self, file_uri: &Url, contents: String) -> anyhow::Result<()> { + let path = file_uri + .to_file_path() + .map_err(|_| anyhow::anyhow!("Invalid file URI: {}", file_uri))?; if !path.is_absolute() { Err(TestServerError::SetFileNotAbsolute(path).into()) } else {