search

haskell / lsp

Haskell library for the Microsoft Language Server Protocol
360 stars 89 forks source link

Migrate to OsPath #445

Closed kokobd closed 1 year ago

kokobd commented 2 years ago

As suggested by @pepeiborra, we have a better alternative to FilePath now. See: https://hasufell.github.io/posts/2022-06-29-fixing-haskell-filepaths.html

And changing the implementation of NormalizedFilePath might greatly impact HLS performance, according to the existing comment. https://github.com/haskell/lsp/blob/a41954e356117d8f5cb64b20e1a9d32e8fc6a884/lsp-types/src/Language/LSP/Types/Uri.hs#L158-L160

I'm planning to:

  1. Modify lsp-types to use OsPath in a fork
  2. Create a hls branch using that fork
  3. Profiling, to verify the performance (especially memory) impact
kokobd commented 2 years ago

Converting between OsPath and FilePath requires the ability to express failure. So we can not preserve the signature of functions like this, if NormalizedFilePath contains OsPath but not FilePath.

fromNormalizedFilePath :: NormalizedFilePath -> FilePath

To maintain backward compatibility, I will create a new type NormalizedOsPath, and change HLS to use it.

michaelpj commented 2 years ago

To maintain backward compatibility, I will create a new type NormalizedOsPath, and change HLS to use it.

I do not think we should worry too much about this. We don't have that many clients, and I'm pretty close to releasing a change that will break essentially the entire package :sweat_smile: I'm happy to keep releasing major versions as we break things

kokobd commented 2 years ago

But keep in mind that using the new filepath requires projects that depends on ghc to upgrade to GHC 9.6, which is not possible for HLS in the near future. NormalizedFilePath is just a standalone type that resides in lsp-types, and is not referenced by other types or functions in lsp. So maybe we should just leave it there to avoid trouble.

Pepe Iborra suggested creating a package filepath-compat which is not tied to newer version of GHC. I think we should restrict usages of filepath-compat to HLS only (it's an application, after all), and don't depend on it here in lsp.

So, I believe we can create NormalizedOsPath in HLS, and move it here when needed.

michaelpj commented 2 years ago

Hmm, could we do a CPP dance to have a NormalizedFilePath that uses OsPath on newer versions of GHC? Or is that just going to be a nightmare?

kokobd commented 2 years ago

could we do a CPP dance

We can, and I think it won't be too hard. But clients will see different API when using different versions of GHC, that might be confusing in my mind. After all, filepath itself just add a new type OsPath, leaving FilePath alone. NormalizedFilePath isn't too much more than a normalized FilePath, so maybe it's reasonable to follow filepath, to conditionally add a NormalizedOsPath on newer GHC versions?

michaelpj commented 2 years ago

Ugh, good point. We could have a single NormalizedFilePath and have filePathToNormalizedFilePath and osPathToNormalizedFilePath on 9.6+ only?

hasufell commented 2 years ago

I'm not sure what NormalizedFilePath is but you can easily maintain a uniform API, e.g. by having only one extraction function NormalizedFilePath -> FilePath. The OsPath codepath would then invoke decodeFS, while the other would probably just unwrap the newtype.

However... do mind that the point of OsPath is to avoid encoding/decoding at the FFI boundary... so you shoudn't convert to or from String yourself in the internal API. That would signal a problem with the approach.