Integrate Cthulhu with VSCode - show types in source code #469
Conversation
|
I haven't looked at the implementation yet, but I'm very excited to see this! I agree with the items on your TODO list that I understand. What's the precise issue about toggling the integration? Would that be something controlled from the VSCode GUI? |
|
I don't think there is any good way to toggle the integration from VSCode, the setting for the types and the warning diagnostics (the orange line saying Unstable Type) is Julia wide. |
Codecov Report
@@ Coverage Diff @@
## master #469 +/- ##
======================================
Coverage 0.00% 0.00%
======================================
Files 9 9
Lines 1433 1512 +79
======================================
- Misses 1433 1512 +79
|
Probably going to be annoying to implement accurate warning ranges so don't bother.
Before when calling `@descend f()` it would only descend into functions called in `f` but not functions called in those functions.
I feel like the redundancy of having the whole function printed in the REPL is more annoying than having the editor jump to the function you've descended into -- that workflow matches the normal debugging experience pretty closely. We could maybe add a At some point I'd also love to do this "properly" and use the debugger UI for this (additionally printing IR/native code in a separate pane to the side), but that's a bigger project. |
Don't revisit already processed callsites and show info when a method definition will not be annotated with VSCode integration.
`get_typed_sourcetext` (specifically the call to `definition`) is slow and this eliminates quite a few calls to it.
Is there a way to have the editor jump to a file without switching focus away from the terminal, I presume this is the desired behavior as otherwise it makes Cthulhu a bit annoying to use. I see that https://code.visualstudio.com/api/references/vscode-api#window.showTextDocument allows the terminal to preserve focus, though this doesn't seem to be exposed in the julia extension. |
They can be disabled in VSCode settings.
There was a problem hiding this comment.
This is looking very good to me. Once others who know more about the vscode side are happy with it, I'm fine with merging.
One question: how VSCode-specific is this, vs being LanguageServer-focused? Your PR here justifiably got a mention on #appreciation on the Julia slack, and one of the only concerns was about the tightness of the coupling. I'm not sure of the answer myself.
Yeah, true. The focus switching would be super annoying for interactive use. |
This doesn't use the LanguageServer at all, I don't think its possible to use LanguageServer here as that would require LanguageServer to load and compile user code. The vscode specific functions are in https://github.com/Zentrik/Cthulhu.jl/blob/master/TypedSyntax/src/vscode.jl and they are |
Undo performance optimisation, move to separate pr.
[skip ci]
I've got this working, I'll make a pr once this is merged as it's largely separate of this pr and not VSCode specific. |
| diagnostics_available_vscode() = isdefined(Main, :VSCodeServer) && Main.VSCodeServer isa Module && isdefined(Main.VSCodeServer, :DIAGNOSTICS_ENABLED) && Main.VSCodeServer.DIAGNOSTICS_ENABLED[] | ||
| inlay_hints_available_vscode() = isdefined(Main, :VSCodeServer) && Main.VSCodeServer isa Module && isdefined(Main.VSCodeServer, :INLAY_HINTS_ENABLED) && Main.VSCodeServer.INLAY_HINTS_ENABLED[] |
There was a problem hiding this comment.
@pfitzseb VSCodeServer is not registered right?
Otherwise we could use package extensions here.
There was a problem hiding this comment.
It is not registered, yes. Do package extensions actually require that though? VSCodeServer is a package and is loaded as such, so the appropriate hooks should fire.
|
I am getting an error now after Pkg.update from this PR. It seems like the compat bounds are incorrect for these packages now, since I have Cthulhu dev'd but the released version of |
| module TestVSCodeExt # stops modules defined in test files from overwriting stuff from previous test | ||
| using Test, PerformanceTestTools, ..VSCodeServer | ||
| @testset "runtests.jl VSCodeExt" begin | ||
| @testset "test_Cthulhu.jl" begin | ||
| include("test_Cthulhu.jl") | ||
| end | ||
|
|
||
| @testset "test_codeview.jl" begin | ||
| include("test_codeview.jl") | ||
| include("test_codeview_vscode.jl") | ||
| end | ||
|
|
||
| # TODO enable this test on nightly | ||
| if false | ||
| @testset "test_irshow.jl" begin | ||
| include("test_irshow.jl") | ||
| end | ||
| else | ||
| @info "skipped test_irshow.jl" | ||
| end | ||
|
|
||
| @testset "test_terminal.jl" begin | ||
| include("test_terminal.jl") | ||
| end | ||
|
|
||
| @testset "test_AbstractInterpreter.jl" begin | ||
| include("test_AbstractInterpreter.jl") | ||
| end | ||
| end | ||
| end |
There was a problem hiding this comment.
@Zentrik What's the whole purpose of duplicating tests here? Can't we include("test_codeview_vscode.jl") only?
There was a problem hiding this comment.
There's some additional work that Cthulhu does when running under VSCode that I wanted to test in more cases than "test_codeview_vscode.jl". The main thing I was worried about was descending into all callees here.
I don't think it's particularly important to run these additional tests given the code has been working for a while, so if you want you can nix it.
There was a problem hiding this comment.
So, it sounds like the purpose of running these duplicated tests was to verify that descending into all callees doesn't lead to any timing issues, right? If so, we might consider removing them. Alternatively, if we decide to keep them, it could be a better design choice to extract the main portion of runtests.jl into a separate file and then include that file in both runtests.jl and test_VSCode.jl.
|
I see that this was merged into master, how can I test it myself? |
|
Call |
|
This is very cool! thanks! |
|
No VSCode only unfortunately. |
Depends on julia-vscode/julia-vscode#3328 for showing types inline, however can be merged in before.
This pr show types in the source code when using the repl in VSCode, e.g.
@descend f(1)with warnings on givesAnd without warnings
There a couple improvements I still want to make to this pr:
includebut then them = @which TSN.unnamedargs(Matrix{Float32}, Int; a="hello")test started failing claimingmiwasnothing)As an example of how to change the colour of the hints in VSCode, I'm using the Monokai theme with the following in my
settings.json