-
Notifications
You must be signed in to change notification settings - Fork 110
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix tide failure in magit buffers. #380
base: master
Are you sure you want to change the base?
Conversation
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.) |
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. |
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 At any rate, it is possible to detect a magit buffer. |
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. This commit makes it so that tide detects that it is running in a magit buffer and aborts.
82810b7
to
6f26bd4
Compare
I've pushed a version that just aborts. I've used |
;; the modes on the file. So when this code runs in such a buffer, | ||
;; `buffer-file-name' is set, even though the buffer is not backed by a file | ||
;; on disk. So we need this check to handle such cases. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.)
;; `buffer-file-name' is set, even though the buffer is not backed by a file | ||
;; on disk. So we need this check to handle such cases. | ||
(when (bound-and-true-p magit-buffer-file-name) | ||
(error "Cannot run tide in a buffer created by magit")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this throwing an error? Nothing really went wrong, it should just disable whatever it can't do, but not throw an error...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"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 not
tide`.
I'm edging now towards display-warning
and returning.
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... |
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.
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? |
My concern with
And |
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 |
And providing this functionality is still problematic. We cannot just do something like what was done for org-mode. Consider the following scenario:
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 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. |
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.