use OsPath in NormalizedFilePath by kokobd · Pull Request #446 · haskell/lsp

kokobd · 2022-07-25T18:26:32Z

Closes #445. Current status:

existing tests pass
breaks backward compatibility due to the necessity to add MonadThrow to the type of some functions
use filepath-compat to avoid CPP, and always use OsPath in NormalizedFilePath
provide two sets of functions for NormalizedFilePath. One for FilePath, another for OsPath

lsp-types/src/Language/LSP/Types/Uri.hs

mergify · 2022-08-14T03:23:07Z

refresh

✅ Pull request refreshed

kokobd · 2022-08-14T03:23:31Z

@mergify refresh

mergify · 2022-08-14T03:24:04Z

refresh

✅ Pull request refreshed

kokobd · 2022-08-14T03:30:59Z

I added a dimension to the test matrix, but Mergify is waiting for the old one. Strange.

kokobd · 2022-08-14T04:59:36Z

lsp-types/src/Language/LSP/Types/Uri.hs


-toNormalizedFilePath :: FilePath -> NormalizedFilePath
-toNormalizedFilePath fp = normalizedFilePath nuri nfp
+toNormalizedFilePath :: MonadThrow m => OsPathCompat -> m NormalizedFilePath


@michaelpj This change leads to about 70 compile errors in HLS. Do we have to fix them one by one?

So the failure here ultimately comes from decodeUtf, right? Which ultimately represents the fact that a filepath is bytes and a URI should be text. So this is a meaningful failure. We could offer a partial version that really throws, but that also seems like a footgun for us in future.

The only other alternative I can see is to have URI/NormalizedUri also contain an OsPath i.e. bytes rather than text. Unclear how bad that would be.

kokobd · 2022-08-29T13:44:51Z

@michaelpj After some rewrites, I'm finally happy with the current version. The most notable change is that now we always store a UTF-8 encoded ShortByteString in NormalizedFilePath, so that toNormalizedFilePath :: FilePath -> NormalizedFilePath is still total.

And please only approve if you feel this is good. Let Mergify squash and merge it for us.

michaelpj

Looking plausible! I worry that maybe this extra encoding/decoding might lose some of the benchmark improvements you were getting?

michaelpj · 2022-08-31T08:24:41Z

.github/workflows/haskell.yml

+    - name: Cabal configure
+      shell: bash
+      run: |
+        if [ ${{ matrix.ospath }} = "true" ]; then


Maybe we should include this as a flag in the package definition? force-ospath or something?

cabal.project

lsp-types/src/Language/LSP/Types/Uri.hs

michaelpj · 2022-08-31T08:27:15Z

lsp-types/src/Language/LSP/Types/Uri.hs

+      Right v' ->
+        return (NormalizedFilePath (internalNormalizedFilePathToUri v') v)
+
+encodeFilePath :: String -> ShortByteString


lsp-types/src/Language/LSP/Types/Uri.hs

michaelpj · 2022-08-31T09:50:53Z

lsp-types/src/Language/LSP/Types/Uri.hs

-fromNormalizedFilePath :: NormalizedFilePath -> FilePath
-fromNormalizedFilePath (NormalizedFilePath _ fp) = fp
+-- | Extracts 'FilePath' from 'NormalizedFilePath'.
+-- The function is total. The 'HasCallStack' constraint is added for debugging purpose only.


This is a little confusing as it immediately calls error! I get the argument: the invariant of NFP is that it contains a correctly encoded string so this should never fail.

However, I also bet we call this a lot. What would happen if we also cached the decoded filepath in NormalizedFilePath? Would it blow out memory usage?

Yes, memory usage is the point of using ShortByteString or OsPath instead of FilePath. Despite the encoding/decoding, both CPU time and memory usage decreased in benchmarks!

I saw most use sites of toNormalizedFilePath just extracts the FilePath and pass it to some IO function, so the CPU is not the bottleneck.

michaelpj · 2022-08-31T09:53:27Z

lsp-types/src/Language/LSP/Types/Uri/OsPath.hs

+normalizedFilePathToOsPath :: MonadThrow m => NormalizedFilePath -> m OsPath
+normalizedFilePathToOsPath = unsafePerformIO' . encodeFS . fromNormalizedFilePath
+
+unsafePerformIO' :: (MonadThrow m, NFData a) => IO a -> m a


michaelpj · 2022-08-31T09:54:08Z

lsp-types/test/URIFilePathSpec.hs


 normalizedFilePathSpec :: Spec
-normalizedFilePathSpec = do
+normalizedFilePathSpec = beforeAll (setFileSystemEncoding utf8) $ do


Does something actually fail if you don't do this?

Yes, default file system encoding ignores the failure. See: https://hackage.haskell.org/package/base-4.17.0.0/docs/src/GHC.IO.Encoding.html#initFileSystemEncoding

So in the real world, it won't throw any exception. (But I want to test the exception)

michaelpj · 2022-08-31T09:58:05Z

lsp-types/src/Language/LSP/Types/Uri/OsPath.hs

+Constructs 'NormalizedFilePath' from 'OsPath'. Throws 'IOException' if the conversion fails.
+-}
+osPathToNormalizedFilePath :: MonadThrow m => OsPath -> m NormalizedFilePath
+osPathToNormalizedFilePath = fmap toNormalizedFilePath . unsafePerformIO' . decodeFS


I'm getting a little confused, would you mind writing a comment somewhere explaining the deal with the various encodings? The idea is something like: we always decode OsPaths using the system encoding into a string, and then we re-encode them as UTF8 to store them in NFP?

I have missed all the discussion in the PR, so I need a comment explaining why we don't do the obvious thing (which would be storing the OsPath inside NormalizedFilePath).

michaelpj · 2022-08-31T10:17:02Z

Oops, I missed the label! oh well, if you want to address any of the comments you can do a follow-up

hasufell · 2022-08-31T14:15:07Z

Looking plausible! I worry that maybe this extra encoding/decoding might lose some of the benchmark improvements you were getting?

Does this still use old directory/base for file operations? Because all of them do encoding/decoding at the FFI boundary. The only thing you'll gain is lower memory footprint due to avoiding String.

Can't say which one has the bigger impact.

kokobd · 2022-08-31T15:33:51Z

Does this still use old directory/base for file operations?

NormalizedFilePath in lsp-types doesn't use directory or some FilePath based IO functions. In HLS, we still use the old directory/base. (HLS can not upgrade before GHC 9.6)

pepeiborra · 2022-09-02T04:47:11Z

lsp-types/src/Language/LSP/Types/Uri.hs

-normalizedFilePath :: NormalizedUri -> FilePath -> NormalizedFilePath
-normalizedFilePath nuri nfp = NormalizedFilePath nuri nfp
+decodeFilePath :: ShortByteString -> Either UnicodeException String
+decodeFilePath = fmap T.unpack . T.decodeUtf8' . BS.fromShort


We can always speed this up with something like utf8-string or rolling our own utf-8 decoding

https://hackage.haskell.org/package/utf8-string-1.0.2/docs/Codec-Binary-UTF8-String.html

pepeiborra · 2022-09-02T05:02:17Z

lsp-types/src/Language/LSP/Types/Uri/OsPath.hs

+osPathToNormalizedFilePath :: MonadThrow m => OsPath -> m NormalizedFilePath
+osPathToNormalizedFilePath = fmap toNormalizedFilePath . unsafePerformIO' . decodeFS


Instead of decodeFS you could use the pure decodeWith and isolate the unsafePerformIO in a single global:

osPathToNormalizedFilePath :: MonadThrow m => OsPath -> Either EncodingException NormalizedFilePath osPathToNormalizedFilePath = fmap toNormalizedFilePath . decodeWith systemEnc {-# NOINLINE systemEnc #-} systemEnc :: TextEncoding systemEnc = unsafePerformIO getFileSystemEncoding

Applied in #453. Thank you!

* use filepath-compat * make NormalizedFilePath use OsPath * remove filepath-compat, use ShortByteString instead * fix ci * enable OS_PATH * upgrade ghc & don't set OS_PATH for ghc 8.6.5 in CI * remove outdated comment * skip a bad ci combination * extract OsPath related CPP to a standalone module * fix OsPath.Compat * add empty NormalizedUri and NormalizedFilePath * add partial function for compatibility & use decodeFS * run stylish-haskell * lock index-state * bump versions of lsp and lsp-types * fix self version bound * always use utf8 ShortByteString in NormalizedFilePath * revert test changes * remove OsPath.Compat * bump lsp-test version according to pvp * add test cases for OsPath * fix test case * add some doc comments

kokobd added 6 commits July 19, 2022 16:39

use filepath-compat

c64e8be

make NormalizedFilePath use OsPath

05c8eb8

remove filepath-compat, use ShortByteString instead

bd0b0e3

fix ci

ec572b1

enable OS_PATH

9d1c18f

upgrade ghc & don't set OS_PATH for ghc 8.6.5 in CI

8fcb58f

michaelpj reviewed Aug 11, 2022

View reviewed changes

lsp-types/src/Language/LSP/Types/Uri.hs Show resolved Hide resolved

lsp-types/src/Language/LSP/Types/Uri.hs Show resolved Hide resolved

lsp-types/src/Language/LSP/Types/Uri.hs Outdated Show resolved Hide resolved

lsp-types/src/Language/LSP/Types/Uri.hs Outdated Show resolved Hide resolved

kokobd marked this pull request as ready for review August 11, 2022 21:53

kokobd added 2 commits August 12, 2022 14:51

remove outdated comment

1a3a463

Merge branch 'master' into feat/os-path

69bd9ef

michaelpj reviewed Aug 12, 2022

View reviewed changes

lsp-types/src/Language/LSP/Types/Uri.hs Outdated Show resolved Hide resolved

skip a bad ci combination

fa6b362

kokobd force-pushed the feat/os-path branch from 85d0390 to 9ebb087 Compare August 13, 2022 09:22

extract OsPath related CPP to a standalone module

fa72e57

kokobd force-pushed the feat/os-path branch from 9ebb087 to fa72e57 Compare August 13, 2022 09:26

fix OsPath.Compat

cd9cec3

kokobd commented Aug 14, 2022

View reviewed changes

kokobd mentioned this pull request Aug 23, 2022

add post jobs to github workflows #451

Merged

kokobd added 3 commits August 23, 2022 17:42

Merge branch 'master' into feat/os-path

1d151e4

add empty NormalizedUri and NormalizedFilePath

f675a60

add partial function for compatibility & use decodeFS

9945200

kokobd force-pushed the feat/os-path branch from 3eaec8e to 9945200 Compare August 25, 2022 13:27

kokobd added 4 commits August 26, 2022 08:38

Merge branch 'master' into feat/os-path

ef2a064

run stylish-haskell

43fc9c8

Merge branch 'feat/os-path' of github.com:kokobd/lsp into feat/os-path

0a88fe8

lock index-state

b4b4b21

bump versions of lsp and lsp-types

7e86651

kokobd mentioned this pull request Aug 26, 2022

Migrate to OsPath haskell/haskell-language-server#3046

Open

kokobd added 6 commits August 26, 2022 22:37

fix self version bound

2fd5f81

always use utf8 ShortByteString in NormalizedFilePath

2b8d61f

revert test changes

80ebd81

remove OsPath.Compat

f46250c

bump lsp-test version according to pvp

488cfe2

add test cases for OsPath

eed3a65

kokobd force-pushed the feat/os-path branch 2 times, most recently from bea0b16 to 02fe4a8 Compare August 28, 2022 05:38

fix test case

c45d471

kokobd force-pushed the feat/os-path branch from 02fe4a8 to c45d471 Compare August 28, 2022 12:04

add some doc comments

ee1702b

kokobd force-pushed the feat/os-path branch from 974d97c to ee1702b Compare August 29, 2022 13:04

kokobd added the merge me label Aug 29, 2022

michaelpj approved these changes Aug 31, 2022

View reviewed changes

mergify bot merged commit 78dc807 into haskell:master Aug 31, 2022

kokobd deleted the feat/os-path branch August 31, 2022 17:41

pepeiborra reviewed Sep 2, 2022

View reviewed changes

kokobd mentioned this pull request Sep 8, 2022

Improve performance of NormalizedFilePath haskell/haskell-language-server#3067

Merged

		osPathToNormalizedFilePath :: MonadThrow m => OsPath -> m NormalizedFilePath
		osPathToNormalizedFilePath = fmap toNormalizedFilePath . unsafePerformIO' . decodeFS

Conversation

kokobd commented Jul 25, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mergify bot commented Aug 14, 2022

✅ Pull request refreshed

Uh oh!

kokobd commented Aug 14, 2022

Uh oh!

mergify bot commented Aug 14, 2022

✅ Pull request refreshed

Uh oh!

kokobd commented Aug 14, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kokobd commented Aug 29, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

michaelpj left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

michaelpj commented Aug 31, 2022

Uh oh!

hasufell commented Aug 31, 2022

Uh oh!

kokobd commented Aug 31, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pepeiborra Sep 2, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

kokobd commented Jul 25, 2022 •

edited

Loading

kokobd commented Aug 29, 2022 •

edited

Loading

pepeiborra Sep 2, 2022 •

edited

Loading