Skip to content

Conversation

@aviatesk
Copy link
Member

@aviatesk aviatesk commented Jul 20, 2020

I this PR I would like to show my ideas to try to make more use of dynamic information so that we can get more enhanced features that LS is probably hard to offer because of its static nature.

For now, I implemented dynamic completions and signature helps, that hopefully don't conflict with the existing LS features or gracefully replace them.

completions

we can offer property/field/dict completions using information from running session.
For example if we have loaded the code below:

struct TY
    v::UInt
    w::Int
end
d = Dict(:a => TY(1, 1), :b => TY(2, 2), :c => TY(3, 3))

then we can get

property completion

Screen Shot 2020-07-21 at 2 54 31

field completion

Screen Shot 2020-07-21 at 2 53 36

dict completion

Screen Shot 2020-07-21 at 2 55 16

They won't conflict with the existing completions from LS and as far as I understand field/dict completions are impossible to be implemented in LS and property completions are hard to offer in a way that works for most cases (like scripting with lots of globals)

signatures help

signature help can be enhanced using user code and type inference; we can dynamically infer the return type and narrow down possible method applicants e.g.:
Screen Shot 2020-07-21 at 2 59 19
(note that Array{Float64, 3} return type gets inferred and also there're only 2 method signatures offered)

The signature helps from dynamic information are more "correct" than those offered by LS when the user code loaded and the type inference succeeds, and in that case we can eagerly replace LS signature helps with the dynamic ones.


This PR is obviously "after JuliaCon" stuff.
I'm also welcome on general discussions about these style of way to use running session.
I hope these dynamic features can live nicely with LS, and we can do yet more interesting like helping linting using dynamic features. But I may miss some points. If we like them, I may also want to mention this kind of enhancement idea in JuliaCon.

@aviatesk aviatesk added this to the Backlog milestone Jul 20, 2020
@aviatesk aviatesk changed the title dynamic completion and signature helps dynamic completions and signature helps Jul 21, 2020
@davidanthoff
Copy link
Member

Yeah, we should definitely do something like this! Without having reviewed the PR in detail, here are some generic thoughts:

  • this is mostly useful for global code, i.e. code not in functions, right?
  • I think the biggest question is whether we implement this like the PR here, where the extension communicates with the REPL process, or whether we add another communication channel between the REPL and the LS, and then return these things via the normal LS protocols. One benefit of the latter approach would be that other users of the LS also benefit from it. Another benefit might be that the LS could potentially do some more intelligent merging of the information from the static analysis and the dynamic REPL state. Downside is maybe a bit more complexity, although I actually think that wouldn't be too bad with the new JSONRPC implementation we have and some work I've been doing over at https://github.com/davidanthoff/CancellationTokens.jl (not finished yet, though).

@pfitzseb
Copy link
Member

this is mostly useful for global code, i.e. code not in functions, right?

It's useful for all values the runtime knows about (i.e. globals), even in a local scope.

I think I'd prefer to always go through the LS, but I feel like we already talked about that at some point and Zac wasn't super happy with that.

@davidanthoff
Copy link
Member

Here is one scenario why I think it would be really good to go through the LS: say a user has the following file

struct Foo
  a::Int
  b::String
end

x = Foo(2, "test")

and then executes that in the REPL. So now we can use the information that x is of type Foo in various places.

Now say the user edits the file and updates the definition of Foo by say adding another field. I think we now really want to make sure that wherever x is used, it picks up the new definition of Foo, even if the new definition of Foo hasn't been executed in the REPL, right? Or not? But if we want to do that, it looks like a non-trivial merge problem of the information that the static analysis gets us and what we get from the REPL, where the REPL can very easily be "behind" in terms of information. It seems to me that if we want to merge that in a good way, we almost have to do that in the LS itself?

I think I'd prefer to always go through the LS, but I feel like we already talked about that at some point and Zac wasn't super happy with that.

So I think one of the main worries was that it would complicate the whole communication story a lot. I think that is true, unless we have something like my cancellation framework. If I get that into the shape that I imagine it should have, then I think it would make this kind of thing not trivial, but much, much simpler. For example, it should make it fairly simple to implement say the request handler for hovers in such a way that at the beginning of the request handler two async tasks are started: one that does the existing static analysis in the LS, and a second one that sends a message to the REPL asking for its information. In this cancellation framework, we would be able to add a timeout to the request to the REPL, so that we essentially have a very simple way of making sure that the REPL request doesn't block us: if it comes back in time, great, we merge information, but if it doesn't (maybe because the REPL crashed, or is blocked, or any other reason), then we'll just return the information from the static analysis side.

@ZacLN
Copy link
Contributor

ZacLN commented Jul 22, 2020

Hiya, I've only read through (but was unable to run/experiment with) the PR so please excuse if I've missed some thing(s).

My main concern relates to how consistency is maintained between what is within a document and what is executed in the REPL to ensure that any given completion is relevant. I don't immediately see how dynamic information for an arbitrary variable x is linked to a symbol within a text document in a reliable fashion, it doesn't seem like module/name is sufficient. This includes dynamic inconsistency issue David refers to above.

These seem like possibly resolvable though fairly complicated issues, but I do feel fairly strongly that information we provide for a file (or project as a whole) should not be state dependent (i.e. on the REPL session).

@pfitzseb
Copy link
Member

Ok, I've pushed a commit to update this and reduce the scope to dynamic completions (no signature help). The feature itself is opt-in and dynamic completion items are marked differently from all LS provided completions to reduce confusion:
image

@davidanthoff
Copy link
Member

Do we need full cancellation token support before we can merge this? I'm worried that the REPL process in general is a very unreliable message processor, and in that case it seems like we should have things like timeouts on requests and general cancel token across the RPC boundary to make sure this doesn't provide a weird experience when the REPL process is blocked. I have ongoing work in https://github.com/davidanthoff/CancellationTokens.jl and julia-vscode/JSONRPC.jl#58 that will make that quite simple, but it will still take a while to finish that.

Just another point: I think this will be especially helpful for notebook kernels, as one tends to have a lot of global code in there.

Finally, I could also still imagine a design where the LS process sends messages to the kernel (REPL or notebook) and then merges things before sending them back to the client. I think once we have cancel token support with timeouts that could also work.

@pfitzseb
Copy link
Member

Do we need full cancellation token support before we can merge this?

No. Requests to the REPL time out after .5s, which makes for a pretty interactive experience.

Just another point: I think this will be especially helpful for notebook kernels, as one tends to have a lot of global code in there.

Agreed. For now this is REPL-only though.

Finally, I could also still imagine a design where the LS process sends messages to the kernel (REPL or notebook) and then merges things before sending them back to the client. I think once we have cancel token support with timeouts that could also work.

Yes, but that comes with a lot of additional complexity, so I'd be in favor of getting this in as-is for now.

@pfitzseb pfitzseb merged commit 5345f3c into master Nov 3, 2021
@aviatesk aviatesk deleted the avi/dynamics branch April 6, 2023 07:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants