Fix Windows by handling Url paths correctly. (#136)

Summary:
Previous implementation was using `Url.path()` where it should use `Url.to_file_path()`

It caused errors like:

```
"Error: `file:///C:/work/bazel_plain/sample.bzl` could not be converted back to a URL\n"
```

More: cameron-martin/bazel-lsp#62

The `Url.path()` is usually `"/" + path to file` which works fine as file path under unix-like systems, but breaks under Windows, where it can be e.g. `"/c:/some/file"`. See
https://en.wikipedia.org/wiki/Uniform_Resource_Identifier#syntax

Switched to using `Url.to_file_path()` which represents actual file path.

In the process:
  - Enabled back Windows tests that at least for my machine started passing;
  - In validate() function not converting from Url to LspUrl and back, just returning to client Url passed in the request.

Pull Request resolved: #136

Reviewed By: KapJI

Differential Revision: D66655476

Pulled By: ndmitchell

fbshipit-source-id: b4590fb27348019424e27ee1d12ffe27d86c6f9f

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"

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()
         }

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;

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<Url> for LspUrl {
     fn try_from(url: Url) -> Result<Self, Self::Error> {
         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<T: LspContext> Backend<T> {
     }
 
     fn validate(&self, uri: Url, version: Option<i64>, 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_click><baz>b</baz>az</baz_click>()
             "#,
         )
-        .replace("{load}", bar_uri.path())
+        .replace("{load}", &uri_to_load_string(&bar_uri))
         .trim()
         .to_owned();
         let bar_contents = "def <baz>baz</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_click><baz>b</baz>az</baz_click>()
             "#,
         )
-        .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>baz</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>baz</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>b</bar>ar<dot>.</dot>star")
-            .display()
-            .to_string();
+            .join("<bar>b</bar>ar<dot>.</dot>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><y_click>y</y_click></y>
             "#,
         )
-        .replace("{load}", bar_uri.path())
+        .replace("{load}", &uri_to_load_string(&bar_uri))
         .trim()
         .to_owned();
 

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 {