Skip to content
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

Normalize file paths before converting to Uri's #205

Merged
merged 8 commits into from
Dec 6, 2019

Conversation

jneira
Copy link
Member

@jneira jneira commented Nov 26, 2019

@jneira
Copy link
Member Author

jneira commented Nov 28, 2019

I am afraid (as @mpickering suspected) that this pr dont ensure all Normalized Uri have the win drive letter upper case.
I wrongly supposed the only way to get it would be FilePath -> Uri -> Normalized Uri but Uri has an Aeson instance (and a Read one!) and it can be produced directly from json (that may have uris with the drive letter lower case)
(UPDATE: he, you can even use directly the Uri constructor, which is public)

So the only way to ensure is to move the code doing the upper case to toNormalizedUri

@jneira jneira changed the title Ensure drive letter is upper case in URIs for windows [WIP] Ensure drive letter is upper case in URIs for windows Nov 28, 2019
NormalizedUri $ T.pack $ escapeURIString isUnescapedInURI $ unEscapeString $ T.unpack t
where (Uri t) = maybe uri filePathToUri (uriToFilePath uri)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure if it is the best way to do it.
filepathToUri main work is to prepare FilePath to convert it to Uri (except making the driver letter upper case)

@jneira jneira changed the title [WIP] Ensure drive letter is upper case in URIs for windows Ensure drive letter is upper case in URIs for windows Nov 28, 2019
@jneira
Copy link
Member Author

jneira commented Nov 30, 2019

I've found that fixing the keys of the VFS with this change makes hlint suggestions appear again in vscode:

image

So it would fix haskell/haskell-ide-engine#1441 and other related issues in hie.

@jneira
Copy link
Member Author

jneira commented Dec 4, 2019

So, you think that actual impl can be acceptable @alanz @cocreature ?
Another alternative ones could be:

  • Move the normalization of windows driver to toNormalizeUri, converting file uris to paths and make the upper case directly instead use filePathToUri
  • A more radical one: use System.FilePath.normalise (that already makes upper case the driver letter) and require paths to be absolute, cause normalise remove bogus .. Uri's only support . for absolute paths, the fact that you can create uris using . at first path component is coincidental as this test case demonstrates:

https://github.com/alanz/haskell-lsp/blob/d76f0cd94e73c2196954c909eec7b6f4443fe668/test/URIFilePathSpec.hs#L77-L83

But this would be a breaking change

@alanz
Copy link
Collaborator

alanz commented Dec 4, 2019

If I recall correctly, fairly early on in HIE development we decided to always use absolute paths, and have nothing relative to cwd, as it is not something you can always rely on, especially if there are multiple packages being served by the same hie server. The project root supplied by the client is used to convert to and from absolute paths, if needed.

And for an index into the VFS, an absolute, normalized path makes sense.

@jneira
Copy link
Member Author

jneira commented Dec 5, 2019

Ok, i've changed the code to use FP.normalise so:

  • The driver letter is converted to upper case
  • The current dir paths are removed

So the resultants uris are more likely to be unique than before

I've updated the tests to cover the above changes and to prove that conversions works with paths using unicode chars.

However i've not add any restriction over relative paths. It would suppose:

  • Throw a runtime error in the actual filePathToUri
  • Or change its signature to FilePath -> Maybe Uri or FilePath -> Either ParseUriError Uri

I think it should belong to another pr cause the objective of this one would be fix uniqueness of uris as map keys.

So for me this could be merged.

@jneira jneira changed the title Ensure drive letter is upper case in URIs for windows Normalize file paths before converting to Uri's Dec 5, 2019
@jneira
Copy link
Member Author

jneira commented Dec 5, 2019

The tests with unicode paths are failing for some environments (or seeds?), investigating

@jneira
Copy link
Member Author

jneira commented Dec 5, 2019

It seems we hit this issue with unicode generation so i've applied the same patch than this pr

@alanz
Copy link
Collaborator

alanz commented Dec 6, 2019

@jneira If you are happy with this it can merge.

@jneira
Copy link
Member Author

jneira commented Dec 6, 2019

@alanz yeah i think it could be merged

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants