Fix tide failure in magit buffers.

[lddubeau]

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]

Isn't this a description of a better strategy instead of hard-wiring it to just magin? That is, test whether the file exists on disk, and abort if it isn't?

[ananthakumaran]

We do support org-mode which uses indirect buffer for chunks of code and might not be linked to a file. But this is very limited and works only for code without any dependencies (aka single file). In the case of magit following the same approach won't work because magit might show old version of code which might not compile without other dependencies.

[lddubeau]

@elibarzilay Besides what ananthakumaran said, there's the issue of how you determine that tide-setup must give up.

So far the discussion in issues here and the comments in the code has distinguished between "buffers backed by files" and "buffers not backed by files", but that's not quite what the issue is. The real issue is whether a buffer has a file name associated with it or not. When we ask Emacs to open a file that does not exit yet, Emacs open a buffer for it, associates the buffer with the file name we asked for, but does not create the file on disk yet. (Note that this specifically precludes using "test whether the file exists on disk" as a test. Otherwise, tide would fail to start in cases where it should.) Though this buffer is not yet backed by a file, it is possible to edit it and get support from tsserver. Eventually, we'll save the file, but whether the buffer has a file that exists on disk or not is not the determining issue. The determining issue is whether or not the buffer has a file name associated with it. tsserver needs to know what the file name of the module is going to be, and so it does not work with buffers that have no file names associated with them.

AFAIK, the standard way to check whether a buffer has a file name associated with it is to check whether buffer-file-name is non-nil. But as I've noted in the comment, magit does set this variable. So by the time tide-setup runs, the buffer has a file name associated with it, and tide-setup does not detect the problem.

Magit sets this variable so that when an old commit is extracted, the editing modes that one would normally expect to see when editing the file are turned on. Since mode selection depends (among other things) on file names, I don't see magit has having any choice here: if it wants to turn on the modes and get sensible results, it has to set buffer-file-name to something sensible.

I generally want to avoid this kind of very narrow case-specific detection, but I'm not seeing a reliable solution here that does not require knowing we're dealing with magit specifically. The more general code path is the code that was already there prior to my changes ( (unless (stringp buffer-file-name) ...) but that code path is not adequate for dealing with magit.

Do you have another suggestion than checking whether the file exists on disk? (As explained above, it won't work.)

[elibarzilay]

Why is this throwing an error? Nothing really went wrong, it should just disable whatever it can't do, but not throw an error...

[lddubeau]

"whatever it can't do" It cannot do anything generally useful. Oh, it is always possible to come up with a small test case where things look fine, but as you start trying more and more complex scenario, tide cannot handle it, primarily because tsserver cannot handle it.

As mentioned in a previous PR comment, error' was expedient for a proof-of-concept. I suppose the mode *could* just silently abort, but that's not something I'm in favor of, as it would leave users puzzling over why typescript-modeis on but nottide`.

I'm edging now towards display-warning and returning.

[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.