goimports on save?

[willie]

This is really the only thing holding me back from Nova full time.

https://godoc.org/golang.org/x/tools/cmd/goimports

[jfieber]

I got a rough version of imports fixup working via gopls this evening and will try and get something PR presentable this week, though practically speaking, likely next weekend. Ideally with format on save as well.

[willie]

Excellent!

[jfieber]

@willie the jump branch of my fork has both imports and formatting commands (in the Editor menu, and command palette) if you want to try them out. They are not extensively tested. They are not yet wired up to be automatic on save. I'm working through proper lifecycle management of gopls server, the Nova LanguageClient, and everything that depends on them when gopls can be flipped on and off. (And I'm super rusty when it comes to JavaScript.)

[willie]

@jfieber I tried them out and can confirm that they mostly work. There is still a lot of other stuff going on (extensions not starting, etc.) that I don't think is your fault.

I'd help if I actually programmed in JavaScript.

[GwynethLlewelyn]

Hi, just to say that I'm working on getting @jfieber's changes all merged with the rest of the code and publish it to Nova's Extensions Library. Sorry for being so slow 😟

[GwynethLlewelyn]

Slow was an understatement, it seems that there is quite a lot going on beneath the hood, and I'm afraid that my meagre JS skills are not enough to work faster. I'm looking at how some extensions achieve the wonders that they do and try to incorporate them into our code. It's not easy, the documentation is driving me to despair...

[jfieber]

I’ve just made my current go extension lab public, but I have no intention of submitting it to the Panic extension catalog. Too many rough edges and I’d rather have a coordinated effort toward a single better Go extension. Also, the way Nova currently handles the workspaces configuration (in the LSP sense) means two extensions using gopls would tend to step on each other’s configuration. My extension uses the syntax definitions from here unmodified, so would also clash in that way.

Getting reliable start/stop/restart of the language server has been challenging, and the code is, ahem, unattractive to say the least, but there is a Restart command for gopls that mostly works, which is required all to often.

There is a built in Install/Upgrade command where you can target a specific gopls version, or just “latest”. That works reliably…on my machine.

There is a random experiment at making tasks, with go mod tidy and go mod vendor as first trials.

The gofmt and imports work more often than not, but not often enough where where I’d consider doing it automatically on save a sane thing. I’m still at the stage of wanting double check the result before saving. There are checkboxes to do it on save, but I honestly don’t recall if it works.

There is a Find Type Definition, because while Nova now has other assorted LSP find commands implemented, that one is curiously missing. I left my implementations of the other Find... commands in place because I push the current location on a stack before jumping, so I can backtrack to where I started. I need to submit “Please add a jump-to stack and a command to unwind it” feature request.

There are lots of gopls preferences knobs, though keeping those synchronized with the ever shifting gopls preferences seems like a path into madness.

The latest release notes for the VS Code Go extension noted that gopls will be enabled by default in the next version. I’d like to spend some time comparing RPC logs on some test cases I have where Nova and gopls develop divergent notions of the current state of the document. gopls will log errors when Nova asks it about something beyond what gopls thinks is the end of the document. I think Nova is more likely to crash gopls sends stuff that doesn’t match Nova’s understanding of the document state.

Anyhow, I’d be happy to help move some bits that do work over, or feel free to harvest anything that looks sane to bring over. But until we pin down why Nova and gopls get out of sync, using an extension based on it will be frustrating at times.

[GwynethLlewelyn]

I'm speechless... thanks for all of your amazing efforts and experiments! I'm still struggling to absorb the many tricks and workarounds you've implemented — that will take me some time (as said, I'm not really a JS programmer...). Clearly, there is quite a lot of things 'under the hood' that completely elude me, and which might explain why @apexskier's TypeScript LSP-based extension seems (for me) to be unnecessarily complicated (I like easy-to-read — and easy-to-debug! — code 😉 ).

There is also an obvious need for Panic to step in and at least explain how they've implemented some things. I haven't checked if you have solved the mysterious issue of why gopls-emitted Markdown for simple hover labels is not parsed by Nova — while @apexskier has no problems whatsoever with that. It seems clear that gopls and other LSP servers work differently; the Google team maintaining gopls are honest about many of the assumptions they made about Microsoft's protocol description, and it seems that some of these assumptions are not 100% correct. On the other hand, it's also plausible that Panic has struggled with the same need on their side: i.e. Panic's assumptions on the more obscure parts of the LSP are not exactly the same as Google's, which very likely causes many of the problems you've encountered.

BTW, talking about assumptions... I'm assuming that what you call 'Nova and gopls being out of sync' is the annoying issue that sometimes gopls gets a shutdown command (or self-submits it, it's hard to track that down...), without explaining why, and so Nova has no other option but to restart the server (and thus the constant need to check if gopls is still running)? I've seen that happening quite a lot, but in most cases (at least I haven't clearly pinpointed a specific case) this is merely annoying — Nova doesn't necessarily crash.

Anyway, to actually answer the OP, it might be a good idea to do the goimports-on-save thing using the technique used by @cloudmanic's own Golang Tools for Nova. He basically bypasses the need for gopls and just implements the on-save commands by calling goimport directly from the disk, similar to how many other non-LSP extensions do. It works, and it isn't plagued with any issues.

[jfieber]

By out of sync, I mean a failure in Text Document Synchronization. Nova sends a textDocument/didChange notification that looks correct, after which the gopls responses to textDocument/documentHighlight requests clearly have a different notion of what the document contains. Nova faithfully renders the highlights according to the Range data in the response, but those ranges do not match where the highlights should be.

Golang Tools for Nova is absolutely the practical way to go today. My extension is just me doggedly trying to make gopls work correctly because long term that is a better strategy. A solid LSP implementation in Nova is a rising tide that lifts all boats. But it kind of sucks right now for go.

[GwynethLlewelyn]

All right, here is an update... I can report some progress on this front.

Panic has been doing a lot of improvements on their LSP support, and it shows: Nova is so much stabler these days when talking to gopls. In fact, I even suspect that the code is now having 'an abundance of caution' when trying to deal with an unresponsive LSP and forcing it to restart more often than necessary. That's something to be looked into...

Anyway, I did some testing, and what seems to be the biggest issue right now is the conversion between Nova ranges and LSP ranges. Why Panic hasn't provided us with a built-in API for dealing with those conversions is beyond my understanding, and I'll most definitely file an issue with them. After all, we know that they internally deal with these things since nowadays Nova is much more precise and consistent at positioning things (hover tips, language tips, etc.) according to what the LSP responds — but that happens only for the under-the-hood, built-in, supported LSP commands.

Now to the point... dealing with formatting/organising imports directly via LSP... which is the point of this issue, anyway...

What I found out was that the code inspired by @apexskier and tweaked by you assumes that the LSP will mostly do text replaces when formatting things. This might be the case for the TypeScript LSP that @apexskier is using. Unfortunately for us, it looks like gopls, by contrast, uses mostly inserts and deletes. If it sends any replaces at all, I have missed them on my testing.

Why does this matter? Well, basically the LspRangeToRange function assumes that the (line; character) pairs for the start and stop positions are always different (or the Range constructor will fail) and that start < end (or, well, the Range constructor will fail, too!). This is a reasonable assumption if all that is going to be done are replaces, and is according to the specs.

However, with inserts/deletes, things are trickier. Hidden on a bit of code in the textEdit specifications, there are a few 'hidden rules' that are very easy to miss, because, well, that's the way Microsoft documents their code. Here it goes:

interface TextEdit {
    /**
     * The range of the text document to be manipulated. To insert
     * text into a document create a range where start === end.
     */
    range: Range;

    /**
     * The string to be inserted. For delete operations use an
     * empty string.
     */
    newText: string;
}

The crucial information is inside the comments!

So... instead of providing an extra field to tell what operation should be performed (replace/insert/delete), Microsoft has established a semantic, contextual approach for differentiating the three cases, namely:

• Range: start < end and newText != "" -> Replace the range with new text
• Range: start === end and newText != "" -> Insert the range with new text at position start (or end, since they're the same)
• Range: start < end and newText === "" -> Delete the range

I was getting a lot of range errors because Nova Ranges have to have start < end (they throw an error otherwise) in many cases, and when ranges were to be inserted, this would fail — meaning that one textEdit would not be applied correctly, and that, in turn, would lead to having all other deltas wrongly applied. The deletion was also problematic in some instances, although it was harder to understand why — my guess is that when the replacement is an empty string, Nova does nothing when using replace(), as opposed to what the LSP specifications expect to happen, i. e., actual deletion.

Why Microsoft chose to do things this way (I personally hate that kind of non-explicit coding of functionality that is indirectly implied... too confusing, too easy to make mistakes, and harder to maintain — what happens if Microsoft decides, one day, that they need a new action beyond replace/insert/delete? How will they implement that without breaking existing LSPs and LSP consumers?) is beyond my understanding, and my only guess is that, somehow, this was how VS Code was originally implemented to deal with changes or versioning or whatever, so it was dragged into the specs...

Fortunately for us, Google has been faithfully reproducing that part of the specs, and, as soon as I figured out that the deltas have to be applied in reverse order (gah! I know, your code already deals with that, but I was still following @apexskier's code, which doesn't worry about the order and just blindly applies them as they're received) — something that Google also documents in a very obscure and non-intuitive way (actually, it's part of their specs where they discuss how to interpret Microsoft's specifications when they are not clear). Also, I also have no idea why Google prefers to do insert/deletes as opposed to doing replaces. There must be a reason for that, but I haven't figured it out yet (maybe gopls can calculate what deltas to send more easily that way...? I'm just speculating).

What all the above means is that it's not enough to blindly convert LSP Ranges into Nova Ranges. The three possible cases have to be figured out in advance and applied differently. Luckily for us, Nova includes separate methods for replace/insert/delete (with different parameters, of course — replace and delete require a range, replace and insert require some text, insert does not use a Range but merely a Position). I've added some new code for a very quick & dirty proof-of-concept. It's horribly inefficient and spews out a lot of junk to the console, but at least it works reasonably well, at least for the examples I've tested so far.

Here is a catch, though. When launching formatFile() from the commands (e. g. menu options, left-click on code, selecting code), the code seems to be working reasonably well. There are some quirks now and then (it's too early to say where they are), but at least the Go source doesn't get mangled — sometimes it simply fails to be properly formatted, that's all. Organising imports works better when selecting the actual import block; when having nothing selected and launching that command from the menu, it often does nothing (but also doesn't mangle the source).

However, when actually saving the file, for some reason that I cannot understand, Nova sends the textDocument/formatting twice — and gopls returns the same sequence of deltas twice. As you might imagine, patching the code with the same sequence of deltas will naturally mangle it beyond repair (yay to Ctrl-Z!). Why this gets triggered twice is really beyond me. I did try to change the code in some way that it would run only once, no matter what Nova thinks, but that obviously didn't work. I suspect that the actual operation takes so long that Nova fires the textDocument/formatting again, for good measure (but why it does it just twice is a mystery; I'd assume that it would try over and over again every X seconds...). Or there are some more obscure instructions somewhere that explain why Nova behaves that way with gopls. Perhaps it's part of the specs. Perhaps it's a timeout problem/race condition — Nova sends the textDocument/formatting command to gopls, gopls does some processing, Nova tires of waiting and sends the same command again, but this time, gopls is finished, and returns all the calculated textEdits — which Nova accepts — but then figures out that there is another textDocument/formatting command on the queue... and proceeds to reply to it again... and because no information was received about any changes, gopls replies with whatever is in its cache for the deltas. Again, all this is my speculation; it could easily be a very obvious flaw in my code that has eluded me, mostly because I've got no real clue about what I'm doing (and increasingly hating JavaScript with a passion...).

Anyway, even Google recommends that editors use gopls for formatting/optimising imports, etc., and claim that the whole point is to get gopls to do everything that all the other 'external tools' do, and, when that happens, it's very likely that gopls will be the Swiss Army Knife and replace whatever 'external tools' Go has — at least when it comes to formatting, pre-processing, etc. I have no idea if this will ever become true or not, but I'd say that CloudManic's approach to launch goimports via the shell is quickly going to become dated and not recommended. It's just that it works now; we're just using a much more complex, incompletely-specified way of doing things. And we're travelling through uncharted waters — there are few Nova extensions using any form of LSP, and those that do, are not really worried about cleaning up the code via the LSP anyway (relying on rock-solid external tools for doing so) — even assuming that other non-Microsoft programming languages have added such functionality anyway. Google, for obvious reasons, is doing their best to keep up with Microsoft's specifications behemoth, but at each step, they need to figure out their own interpretation of Microsoft's confusing rules and work out a way to implement them according to their interpretation — which might differ from Microsoft's own...

[jfieber]

@GwynethLlewelyn I got word from Logan that https://github.com/jfieber/GoBang.novaextension has been fixed and should ship in the next Nova release. So I'm hitting Check for Updates with a slightly manic pace. To be honest, I had just hit pause on my extension work, pending that fix, because I suspect some other issues might be related, and the workaround hides a number of desired features, like an out-of-the-box organize imports, no extension coding required. Theoretically.

But some delights have shown up in the meantime. Not sure which Nova+gopls it started in, but completions now add missing imports automatically if appropriate, for standard library stuff at least. Was quite startled the first time that happened.

[GwynethLlewelyn]

Ohhhh... now that's very interesting news! Crossing my fingers now!