Fix tide failure in magit buffers.

[lddubeau]

trafficstars

When a user uses magit to extract old commits of files, magit creates buffers for these commits. These buffers are not backed by actual files. Tide is turned on in these buffers. Prior to this commit tide would report errors whenever it tried to contact tsserver to work on one of these buffers.

Fixes #378

@elibarzilay Please try this out and let me know how it goes. Prior to this patch, I was able to reproduce your error. After the patch I can't.

Note that I don't use org-mode in a way that involves tide, and I don't see any tests that specifically exercise the code I've changed, so I cannot tell whether I've broken anything that used to work.

[lddubeau]

I've noticed the test failure. It looks like a transient failure while testing with one version of Emacs. (The test does not actually even start for that version.)

[ananthakumaran]

If I understand correctly, we are using the same strategy we use for org files (aka send the full content), but won't this cause issue?. Especially, if I look at an old commit, it will start to send old code to tsserver and that might not compile etc.

[lddubeau]

You're right. I goofed. I knew that looking at old revisions along with the working directory might cause disconnects due to symbols having been added or removed, but I misunderstood what is going on with tsserver when the code path for tide-require-manual-setup is taken. I was hoping at least for some level of support, but it seems that won't be possible.

At any rate, it is possible to detect a magit buffer. tide-setup could essentially abort if it detects that it is in such a buffer. I looked at magit's code and did not see there any clean way to work around the issue.

[lddubeau]

I've pushed a version that just aborts. I've used error to abort because that was expedient for a proof-of-concept. I'm not terribly keen on using something like message because everybody and their brother uses message and thus anything reported there tends to be lost in the noise.

[elibarzilay]

I don't know enough about what the tsserver can and cannot do, but isn't there some mode where it can provide some information about some piece of code without needing --or-- affecting other files?

In any case, throwing an error is not something that should happen when things work normally, so using an error here is wrong. Ideally, it would disable only stuff that cannot be done reliable as above...

[ananthakumaran]

but isn't there some mode where it can provide some information about some piece of code without needing --or-- affecting other files?

It might work for some cases, but in majority of the cases, it's not going to work. Think about what would happen if I open a commit made last year. The dependency files might or might not be there. Even if we start an isolated tsserver for this single file, there is no guarantee it would work (unless we checkout all the files from that specific commit)

I also don't see what's the usefulness of tide in magit buffers. If I want to look at the current code, I would just do Ctrl-Enter, which will take to the real current file.

In any case, throwing an error is not something that should happen when things work normally, so using an error here is wrong. Ideally, it would disable only stuff that cannot be done reliable as above...

I agree with this. this is not an error, because as a user, there is no way to stop this (unless I start to check if it's a magit buffer before calling tide-setup). Perhaps just a message would be sufficient?

[lddubeau]

Perhaps just a message would be sufficient?

My concern with message is that it is easy for a message to get lost in the noise. Message works mostly okay as the last step of an interactive command, but here tide-setup is one function called among the dozens of functions that are called to setup the modes. In my not-yet-committed code I now have:

  (when (bound-and-true-p magit-buffer-file-name)
    (display-warning 'tide "Cannot run tide in a buffer created by magit"
                     :warning)
    (cl-return-from tide-setup))

And tide-setup is now defined using cl-define.

[elibarzilay]

Limited functionality: what I'm talking about are things that should be useful without the context of other files, for example, highlighting identifiers and completions based on the current buffer are still useful. (More for the former, since it's useful when viewing code, editing is unlikely to happen, of course).

As a sidenote, using cl-return-from is not a popular thing in lisp code, using a conditional around the code is way more common...

[lddubeau]

for example, highlighting identifiers and completions based on the current buffer are still useful.

And providing this functionality is still problematic. We cannot just do something like what was done for org-mode. Consider the following scenario:

1. Suppose you're looking at foo.ts in Emacs, assume that tide is enabled on that buffer. When you opened this file tide sent the contents of the file to tsserver and told it that this was foo.ts.

2. Then you ask magit to check for an old revision of this file, so magit creates a new buffer called (for instance) foo.ts~master^~. Going by the old strategy, the file associated with this buffer would also be foo.ts. So tide sends the file's content to tsserver and tells it that this file is foo.ts. As far as tsserver is concerned, this foo.ts file is the same foo.ts file as the file in the first step above. Emacs has two buffers with different versions of foo.ts, but tsserver knows only one foo.ts, and as far as it is concerned, the content of that file has been completely replaced.

Hilarity ensues. If you go back to your first buffer and highlight identifiers, the locations will all be messed up.

It is possible to isolate the two versions by giving a different file name to tsserver when sending the content of foo.ts~master^~. The fake file name has to be chosen carefully though. If it is such that the new fake file name is taken by tsserver to be in the same project as the original foo.ts, then strange errors may crop up. A name could be selected so that the new file is associated with an inferred project, but that would create other problems. (Note that I do not know of any sanctioned method by which tsserver can be forced to associate a file with an inferred project. Giving a file a name which puts it outside all projects known to tsserver does work, but it is a hack.)

I've actually tried the workarounds I described above, and ran into trouble. Moreover, one problem with the workarounds is that when they fail, the failure is obscure to the end user. A random tide user faced with a tide failure in a magit buffer where tide has played some clever trick with file names won't readily know what is going on.