Subscribe to navigation events on open files

[JoeyScarr]

Use that to navigate if available, otherwise fall back to requesting navigation directly from the analysis server.

[JoeyScarr]

This is the change I mentioned in #3620.

[DanTup]

I think this looks fine (with two minor nits/comments), although I think it may introduce a race that could be hit if the server is busy that might result in inconsistent results:

• Notification is sent from server, client caches it
• Server gets busy so it's not responding to requests
• Modify a file (notification is sent to server)
• Try to navigate - it will use the stale results from the previous notification (since the server hasn't sent the new one yet)

With the previous code, the request would go to the server and be queued/async, so should return the latest results.

I'm not sure this is an issue worth worrying about though (both because it seems unlikely, and because almost all users are using LSP), so I don't think it blocks landing this.

I'll wait for your response to the comments below, but otherwise I think we can merge this. Thanks for the contribution!

[JoeyScarr]

I think this looks fine (with two minor nits/comments), although I think it may introduce a race that could be hit if the server is busy that might result in inconsistent results:

• Notification is sent from server, client caches it
• Server gets busy so it's not responding to requests
• Modify a file (notification is sent to server)
• Try to navigate - it will use the stale results from the previous notification (since the server hasn't sent the new one yet)

With the previous code, the request would go to the server and be queued/async, so should return the latest results.

I'm not sure this is an issue worth worrying about though (both because it seems unlikely, and because almost all users are using LSP), so I don't think it blocks landing this.

I'll wait for your response to the comments below, but otherwise I think we can merge this. Thanks for the contribution!

There is another issue I think, which is that if the DAS is slow to send back the navigations for a file after it's modified, the stored navigation offsets will be wrong. This could be solved by tracking offset changes from edits and taking them into account when finding the right navigation link to use, but will leave up to you to decide whether you care.

[DanTup]

There is another issue I think, which is that if the DAS is slow to send back the navigations for a file after it's modified, the stored navigation offsets will be wrong. This could be solved by tracking offset changes from edits and taking them into account when finding the right navigation link to use,

Yeah, that's the same issue really. Another option could be to just blow away the cached data whenever a file is modified, so in that case it'll hit the server. It could be done with a listener on workspace.onDidChangeTextDocument in the constructor, similar to the existing open/close calls.

I'm happy to make that change after merging this, or you're welcome to add (or, if you think that will introduce problems, let me know).

[JoeyScarr]

There is another issue I think, which is that if the DAS is slow to send back the navigations for a file after it's modified, the stored navigation offsets will be wrong. This could be solved by tracking offset changes from edits and taking them into account when finding the right navigation link to use,

Yeah, that's the same issue really. Another option could be to just blow away the cached data whenever a file is modified, so in that case it'll hit the server. It could be done with a listener on workspace.onDidChangeTextDocument in the constructor, similar to the existing open/close calls.

I'm happy to make that change after merging this, or you're welcome to add (or, if you think that will introduce problems, let me know).

Sounds fine, feel free to add that. I've been using the plugin with just this patch and the issue doesn't seem to crop up too often, so I'm not too fussed either way. Thanks for the review!

[DanTup]

I decided not to bother with the change above, as I don't think it would be 100% reliable anyway. Eg:

• you modify document (1)
• we clear the local cache
• you modify document (2)
• we clear the local cache
• server sends notification from edit (1)
• you Go-to-Definition, and use the stale results

So since we can't really guard against it reliably, I'm not sure it's worth trying to cover a subset of those cases. The chance of it occurring seems fairly slim anyway (and ofc this code will ultimately go away).

Thanks for the contribution!