Make encode work for zipfile deps #1327

ericdallo · 2022-10-15T21:50:10Z

ericdallo · 2022-10-15T21:57:03Z

cli/integration-test/integration/definition_test.clj

+
+(deftest definition-external-dependency-zipfile-scheme-escaped-uris
+  (h/delete-project-file "../../.lsp/.cache/")
+  (h/set-escape-uris! true)


I thought about passing a param but would need lots of changes saying that the URI should be encoded, so a dynamic var was less worst 😅

Would with-redefs work? That'd let you avoid having to remember to set the var back to false at the end of the test.

I think so, for a moment I thought it was not thread-safe

mainej · 2022-10-16T00:08:37Z

Here's my hypothesis about how clojure-lsp works (before this patch) and why the patch fixes things. @ericdallo, do you agree with this thought process?

At startup we give clj-kondo the classpath, but not specific file names. It analyzes the paths, returning analysis with unescaped regular file and jar file names. We convert those file names to URIs, leaving them unescaped. The project internal files are all fine because their URIs are the same whether escaped or not. But the DB's references to external files use unescaped URIs even though the client plans to use escaped URIs. (This is arguably the first part of the bug, but the dep-graph saves us from some problems, as I'll explain in a moment.)

So suppose you're in an internal file, on a usage of an external var and you want to go to its definition. We use the dep graph to go from the var's namespace to the (unescaped) jar file URI. We tell the editor to open that URI, which it does. It sends a didOpen for that external file, giving us an escaped URI.

We then do a little dance with clj-kondo which wants filenames not URIs. We unescape the URI (in shared/uri->filename) and pass that filename over to clj-kondo. clj-kondo returns the analysis. We convert the filename back into a URI (with shared/filename->uri). But we leave it unescaped, which causes the bug: we update the DB, but keep using the unescaped jar file URI. So, the DB is still full of unescaped URIs.

So, now, you're in an external file on a usage of a var defined in another external file and you want to go to that definition. When the client makes this request, it provides the source file's ESCAPED URI. But we don't have any analysis for the escaped URI, only for the unescaped URI. We can't look up the var usage, and therefore can't tell the client where it's defined.

That's why the second navigation is broken. @ericdallo do you feel the integration tests sufficiently cover this two-step navigation (from an internal file, to a jar file, to a second jar file)?

So to summarize, after the change in this PR things are a bit different. At startup and in didOpen we saved escaped jar file URIs. There's a minor change on the first navigation (internal file to jar file). We now tell the client to open an escaped URI. (Does that work? I assume so, but I'll test it to make sure.) On the second navigation (jar file to second jar file) we actually have analysis, so we can provide a destination.

I see why you're hesitant about the way this is being fixed. The fix (= "zipfile" (:dependency-scheme client-settings)) has two downsides:

It doesn't fix the bug for any client that escapes URIs this way, unless they also use the zipfile scheme.
It breaks any other clients that do use zipfile but don't escape.

I guess this is OK as a quick fix. We think that coc.nvim is the only client that escapes this way, and also think it's the only client that uses zipfiles. So as for the first downside, we're only introducing a bug if future clients escape but don't use zipfiles. Similarly, for the second downside, we're only introducing a bug if future clients use the zipfile scheme but don't escape.

For a longer term fix, I suppose we've forced all the clients to include :dependency-scheme "jar" in their initialization options, just so that coc.nvim can omit :dependency-scheme "zipfile" from its options. Should we do that again, but this time force coc.nvim to include :uri-scheme "escaped-path" or something like that? @NoahTheDuke are you amenable to that?

This file name code is so delicate, with so many little hacks. Someday we should take a step back, document all the ways URIs and filenames are used (by clients and by clj-kondo on Windows and UNIX-likes), and come back to it with fresh eyes.

mainej · 2022-10-16T00:19:23Z

Does this need a bump to clojure-lsp.db/version? Or do we tell coc.nvim users to delete their cache?

mainej · 2022-10-16T00:26:31Z

I apologize, but I can't test this... something is wrong with my nvim. When I try to open an external definition it always takes me to a blank buffer. @dharrigan would you mind confirming that a build from this branch fixes thing for you?

dharrigan · 2022-10-16T05:07:45Z

Hi,

Unfortunately, I don't think it works. I've included a window recording. I'm opening up the main.clj file, waiting for analysis to complete, then gd'ing into validate. Now nothing is shown.

mainej · 2022-10-16T18:08:47Z

@dharrigan your video answers a question buried in my long comment.

We now tell the client to open an escaped URI. Does that work?

The answer, apparently, is no. coc.nvim gives clojure-lsp escaped URIs, but cannot accept them in return. (I ❤️ all the clients, but hacking around individual clients is never a fun part of working on an API.)

What this means, I think, is that clojure-lsp has to return unescaped URIs to all clients. I see two paths to doing this. First, we could unescape URIs as they're leaving clojure-lsp, in our responses. Alternatively, we could escape URIs as they're entering clojure-lsp, which is what we were doing before by calling uri->filename on everything.

I've been pushing for the DB to contain URIs in the same format that the clients send, which would suggest we should take the first option—unescape URIs on the way out of clojure-lsp. But that no longer feels like the best approach. First, we know that unescaping URIs on the way in works—that's how the system worked before. Second, looking through the spec, it's much easier to unescape the client's requests. The server's responses include URIs in many many deeply nested locations. On the contrary, almost all of the client's requests are uniform—there's only one URI, and it's at [:text-document :uri].

@ericdallo I think we can write an integration test to cover these scenarios, but I'm not confident that seeing a green integration test is sufficient. There's too much subtlety in how individual clients work. We need to reproduce this for ourselves, in nvim. I'm going to start from scratch getting my nvim to work. Maybe this time I'll try @dharrigan's config.

I have time to work on this today, so I'll do that. @ericdallo I'll start from this branch. Mind if I push changes to it?

ericdallo · 2022-10-17T00:23:18Z

Sorry for the delay and thanks for the complete explanation @mainej that's exactly what I found after some debugging.

That's why the second navigation is broken. @ericdallo do you feel the integration tests sufficiently cover this two-step navigation (from an internal file, to a jar file, to a second jar file)?

I think so, we call hover on the opened jar which is similar to find-definition from a second jar, WDYT?

This file name code is so delicate, with so many little hacks. Someday we should take a step back, document all the ways URIs and filenames are used (by clients and by clj-kondo on Windows and UNIX-likes), and come back to it with fresh eyes.

For sure, I'd love to see all upstream tools using URIs, starting from clj-kondo, then we wouldn't need to do all this magic probably

Does this need a bump to clojure-lsp.db/version? Or do we tell coc.nvim users to delete their cache?

Yes, sounds safer to bump db version, I don't see a problem with that.

I've been pushing for the DB to contain URIs in the same format that the clients send, which would suggest we should take the first option—unescape URIs on the way out of clojure-lsp. But that no longer feels like the best approach. First, we know that unescaping URIs on the way in works—that's how the system worked before. Second, looking through the spec, it's much easier to unescape the client's requests. The server's responses include URIs in many many deeply nested locations. On the contrary, almost all of the client's requests are uniform—there's only one URI, and it's at [:text-document :URI].

Agreed, My first idea was to escape as we did before, but then I thought the same, save in DB as client sends, but on a second thought, this could be dangerous as we have lots of outputs and URIs that come from other upstream like kondo, so it sounds safer to just do the same we did in the past

@ericdallo I think we can write an integration test to cover these scenarios, but I'm not confident that seeing a green integration test is sufficient. There's too much subtlety in how individual clients work. We need to reproduce this for ourselves, in nvim. I'm going to start from scratch getting my nvim to work. Maybe this time I'll try @dharrigan's config.

Agreed, I'll find some time this week to do the same.

I have time to work on this today, so I'll do that. @ericdallo I'll start from this branch. Mind if I push changes to it?

Not at all, feel free to do it

NoahTheDuke · 2022-10-17T00:57:33Z

coc.nvim gives clojure-lsp escaped URIs, but cannot accept them in return

This seems like a coc.nvim bug unless it's how the LSP spec is written. I can reach out to them and see about fixing or working around it.

mainej · 2022-10-17T01:46:31Z

I think so, we call hover on the opened jar which is similar to find-definition from a second jar, WDYT?

@ericdallo ... I think it's different. hover doesn't return a URI, but find-definition does. We need to be checking that URI is in the right format.

This seems like a coc.nvim bug unless it's how the LSP spec is written.

@NoahTheDuke I couldn't find anything about escaping in the spec. So, predictably since it's unspeced, you get the free-for-all that we're seeing. My guess is that each client and server is just doing whatever their URL lib does, including to some extent clojure-lsp. Anyway, yes, coc.nvim may be doing it wrong, but so are others. For example, I discovered today that Calva escapes the entire jarfile including the second colon "jar:file%3A///some/path/some.jar%21/some/file.clj". This is totally different than other clients, but I can't even say it's wrong. It's invalid to have a colon in a URI scheme, so jar:file://... is technically an invalid URI. Such a mess.

mainej · 2022-10-17T01:47:44Z

I eventually got nvim working and was able to reproduce the problem. @dharrigan and/or @snoe will you test this branch? The patch in its current form fixed the problem for me.

mainej · 2022-10-17T02:10:17Z

cli/src/clojure_lsp/server.clj

@@ -459,7 +494,7 @@
  ;; about which messages are sent when probably needs to be handled in lsp4clj.
  ;; https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#initialize
  (handler/initialize components
-                      (:root-uri params)
+                      (normalize-uri (:root-uri params))


I'd like some help thinking about this change. I think that this means that the root-uri never contains %3a anymore, which means that :encode-colons-in-path? is always false, and therefore should be removed.

@LuisThiamNye you introduced :encode-colons-in-path? in #439. I don't know if you still follow this project, but if you do, can I recruit your help? Do you remember when you saw colons encoded as %3a in the root-uri? What OS/editor/client/repo combination was it? Can you test this branch there?

I think :encode-colons-in-path? was for Windows, but I don't have access to a Windows machine. @ikappaki, you've done the most recent Windows maintenance. Do you have any insight or concerns about this change?

Apologies for bringing you both in without much context. As a really quick summary, the intention of this line and the other changes to server.clj is to put every URI we receive from a client into a format that A) will match the URIs we get from clj-kondo, and B) can be used by the editors to open files, especially jar files. By doing this uniformly in server.clj, we don't need as much special logic in other places to convert URIs into particular formats.

@ericdallo I did all my testing with (normalize-uri (:root-uri params)), so I know it works on a Mac with emacs-lsp, Calva and coc.nvim. If we decide to revert this line, we have to remember to test all those again.

👍

I guess project-uri will never come with colons but I may be wrong.

Do you remember when you saw colons encoded as %3a in the root-uri?

According to this comment, I observed this with Calva on Windows to denote the root drive (c: → c%3A).

Thanks @LuisThiamNye. The unit tests already cover c: being encoded as c%3a. I hope that means Calva will work on Windows, but if you or @ikappaki have a chance to confirm, I'd appreciate it.

Hi @mainej,

I had a quick play with this branch and Calva, and navigation from the clojure-lsp project to external libs appears to be working as normal. Do you have a particular test case you want us to try out?

Amongst other things, I've tried the following which I think is relevant since I'm jumping into jars:

Open cli/src/lcojure_lsp/server.clj and right click on timbre, goto definition, which gets me into the timbre-5.2.1.jar

Right click on reduce keyword, goto definition, it gets me into the clojure core.clj jar.

Thanks

@ikappaki thanks a ton! That's really helpful. Your test is basically what I was thinking. I'd sort of like to know if you can jump from one timbre file to another, but I think that's the same as jumping from a timbre file to the clojure.core file. If you get a chance to check that, I'd appreciate it, but it's not crucial.

Hi @mainej,

I confirm I can jump from cli/src/lcojure_lsp/server.clj -> ~\.m2\repository\com\taoensso\timbre\5.2.1\timbre-5.2.1.jar!\taoensso\timbre.cljc -> ~\.m2\repository\com\taoensso\timbre\5.2.1\timbre-5.2.1.jar!\taoensso\timbre\appenders\core.cljc with no issue (also I can jump to definitions within the same timbre file).

Brilliant! Thanks @ikappaki 🙏

ericdallo · 2022-10-17T02:13:15Z

@ericdallo ... I think it's different. hover doesn't return a URI, but find-definition does. We need to be checking that URI is in the right format.

Makes sense, testing a definition for a jar or anything that will return a URI sounds better indeed

This ensures that the handlers receive URIs that match the URIs in the db, including those generated by converting clj-kondo filenames into URIs. I think that with this change, `:encode-colons-in-path?` is always false, and should therefore be removed.

ericdallo · 2022-10-17T11:42:43Z

lib/src/clojure_lsp/shared.clj

@@ -529,3 +531,13 @@
  expects Clojure style JSON, or when a map needs to be round-tripped from
  clojure-lsp to the client and back without case changes."
  lsp.responses/preserve-kebab-case)
+
+(defn normalize-uri-from-client [uri]


I wonder if we should have this as a lsp4clj flag, it seems to me that all servers could have this encode problem from clients, so a easy flag to enable that or not may be a good idea

Agreed, it's a shared problem. But since lsp4clj doesn't know about the structure of any of the inbound LSP messages, it'd be hard for it to do the conversion itself. This helper function would definitely be a candidate for clojure-lsp/lsp4clj#14 though.

This is another thing that might have been easier if we'd modeled the message handlers as routes instead of multimethods.

ericdallo commented Oct 15, 2022

View reviewed changes

mainej mentioned this pull request Oct 16, 2022

Instruct clojure-lsp about how coc.nvim conveys URLs NoahTheDuke/coc-clojure#8

Open

mainej reviewed Oct 17, 2022

View reviewed changes

ericdallo and others added 3 commits October 16, 2022 19:19

Make encode work for zipfile deps

cf30df4

Pass db, not db*, through call-hierarchy

133e988

Normalize URIs that clients send

c50b616

This ensures that the handlers receive URIs that match the URIs in the db, including those generated by converting clj-kondo filenames into URIs. I think that with this change, `:encode-colons-in-path?` is always false, and should therefore be removed.

mainej force-pushed the fix-encoding branch from 42ee3c0 to c50b616 Compare October 17, 2022 02:20

mainej added 2 commits October 16, 2022 19:39

Try to fix tests for Windows

7427962

Try again to fix tests for Windows

dfbf0bf

ericdallo commented Oct 17, 2022

View reviewed changes

ericdallo added 3 commits October 18, 2022 13:34

Replace set with with-redefs

59918a3

Add find-definition to integration test

4ca5da6

Fix tests

0195327

ericdallo merged commit 2b07890 into master Oct 18, 2022

ericdallo deleted the fix-encoding branch October 18, 2022 22:30

ikappaki mentioned this pull request Nov 24, 2022

Fix Calva's escaped URIs rename issue #1389

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make encode work for zipfile deps #1327

Make encode work for zipfile deps #1327

ericdallo commented Oct 15, 2022 •

edited

Loading

ericdallo Oct 15, 2022

mainej Oct 16, 2022

ericdallo Oct 17, 2022

mainej commented Oct 16, 2022

mainej commented Oct 16, 2022

mainej commented Oct 16, 2022

dharrigan commented Oct 16, 2022

mainej commented Oct 16, 2022

ericdallo commented Oct 17, 2022

NoahTheDuke commented Oct 17, 2022

mainej commented Oct 17, 2022

mainej commented Oct 17, 2022

mainej Oct 17, 2022

ericdallo Oct 17, 2022

LuisThiamNye Oct 17, 2022

mainej Oct 17, 2022

ikappaki Oct 18, 2022

mainej Oct 18, 2022

ikappaki Oct 18, 2022 •

edited

Loading

mainej Oct 18, 2022

ericdallo commented Oct 17, 2022

ericdallo Oct 17, 2022

mainej Oct 17, 2022 •

edited

Loading

Make encode work for zipfile deps #1327

Make encode work for zipfile deps #1327

Conversation

ericdallo commented Oct 15, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mainej commented Oct 16, 2022

mainej commented Oct 16, 2022

mainej commented Oct 16, 2022

dharrigan commented Oct 16, 2022

mainej commented Oct 16, 2022

ericdallo commented Oct 17, 2022

NoahTheDuke commented Oct 17, 2022

mainej commented Oct 17, 2022

mainej commented Oct 17, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ikappaki Oct 18, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ericdallo commented Oct 17, 2022

Choose a reason for hiding this comment

mainej Oct 17, 2022 • edited Loading

Choose a reason for hiding this comment

ericdallo commented Oct 15, 2022 •

edited

Loading

ikappaki Oct 18, 2022 •

edited

Loading

mainej Oct 17, 2022 •

edited

Loading